ParameterSet: Use python3 to run tests

[smuzaffar]

This is needed in order to drop the next set of python2 based modules ( cms-sw/cmsdist#7112 ).
These are technical changes and should not break any thing as unit tests are working in PY3 IBs where python is python3.

[smuzaffar]

please test

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34477/23931

• This PR adds an extra 12KB to repository

[cmsbuild]

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

• FWCore/ParameterSet (core)

@makortel, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @wddgit this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

[makortel]

I suppose this means it is time to "fix" the unit test that fails in FWCore/ParameterSet for python 3.

[cmsbuild]

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-29c069/16788/summary.html
COMMIT: c84edd7
CMSSW: CMSSW_12_0_X_2021-07-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34477/16788/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test TestFWCoreParameterSetDriver had ERRORS

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 38
• DQMHistoTests: Total histograms compared: 2786302
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2786279
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
• Checked 160 log files, 37 edm output root files, 38 DQM output files
• TriggerResults: no differences found

[makortel]

Ok, this turned out to be more complicated than I thought (I was expecting to fix #28703 but the shell script stops at first error, and the first failure hid many others)

I got to 1aacba4cb61, but there are still two failures that I couldn't figure out quickly what to do

======================================================================
ERROR: testReplaceIfHeldDirectly (__main__.TestModuleCommand)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../CMSSW_12_0_X_2021-07-12-2300/python/FWCore/ParameterSet/SequenceTypes.py", line 2091, in testReplaceIfHeldDirectly
    s3._replaceIfHeldDirectly(~m1, m2)
  File ".../CMSSW_12_0_X_2021-07-12-2300/python/FWCore/ParameterSet/SequenceTypes.py", line 459, in _replaceIfHeldDirectly
    if original in self._tasks:
  File ".../CMSSW_12_0_X_2021-07-12-2300/python/FWCore/ParameterSet/OrderedSet.py", line 42, in __contains__
    return key in self.map
TypeError: unhashable type: '_SequenceNegation'

Here I don't understand how _ModuleSequenceType._tasks works even in the first place given that it puts "random (unhashable) stuff" into OrderedSet that uses those as keys to dict.

======================================================================
FAIL: testAllowed (__main__.testTypes)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../CMSSW_12_0_X_2021-07-12-2300/python/FWCore/ParameterSet/Types.py", line 1983, in testAllowed
    self.assertRaises(ValueError,lambda: setattr(p1,'aValue',PSet()))
AssertionError: ValueError not raised by <lambda>

i.e. the setattr() here

cmssw/FWCore/ParameterSet/python/Types.py

Line 1983 in 1aacba4

self.assertRaises(ValueError,lambda: setattr(p1,'aValue',PSet()))

should fail but it doesn't. Same occurs with python2, in master this "test failure" gets ignored because the setattr() is called by the testAllowed() function body instead of the self.assertRaises() function. @Dr15Jones, could you take a look?

How should we proceed in general? Fixing these errors by many people on top of this PR plus my commit starts to become cumbersome.

There are also bunch of deprecation warnings, but I'd leave those to be addressed a little bit later time (e.g. I'd be tempted to not care if the fixes work with py2), but still for 12_0_0.

[davidlange6]

specifically for @makortel question on random unhashable stuff - tasks are supposed to be of type Task. So, put a check for that (just as there is a check for appending to self._tasks)

-        if original in self._tasks:
+        if isinstance(original,Task) and original in self._tasks:

[davidlange6]

about the setattr I see that

>>> p1.aValue=1.3
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/dlange/CMSSW_12_0_X_2021-07-13-2300/python/FWCore/ParameterSet/Mixins.py", line 284, in __setattr__
    self.__dict__[name].setValue(value)
  File "/dlange/CMSSW_12_0_X_2021-07-13-2300/python/FWCore/ParameterSet/Types.py", line 45, in setValue
    v = self.__type(value)
  File "/dlange/CMSSW_12_0_X_2021-07-13-2300/python/FWCore/ParameterSet/Types.py", line 142, in __call__
    raise RuntimeError("Cannot convert "+str(value)+" to 'allowed' type")
RuntimeError: Cannot convert 1.3 to 'allowed' type

seems not to throw a ValueError but rather a RuntimeError.

and second, that reseting aValue as a PSet removes the original requirement

>>> p1.aValue=cms.PSet()
>>> p1.aValue=1
>>> p1.aValue=1.3

Both of these look to be true also in python2. (eg, in pre3) -- A potentially better test

self.assertRaises(RuntimeError,lambda: setattr(p1,'aValue',1.3))

[davidlange6]

fixing that I"m still left with testTaskPlaceholder and testTask errors that I would have thought @makortel 's commit fixes - but perhaps we have another set non reproducibility issue to find?

[makortel]

specifically for @makortel question on random unhashable stuff - tasks are supposed to be of type Task. So, put a check for that (just as there is a check for appending to self._tasks)

-        if original in self._tasks:
+        if isinstance(original,Task) and original in self._tasks:

Thanks @davidlange6, that would indeed work. I'm still confused though why cms.Task() is hashable but _SequenceNegation is not. The _SequenceNegation inherits from _UnarySequenceOperator, which is also not hashable, and that inherits from _BooleanLogicSequenceable, which is hashable.

Ah, now I found the reason. _UnarySequenceOperatordefines __eq__() but not __hash__(), and

If a class does not define an __eq__() method it should not define a __hash__() operation either; if it defines __eq__() but not __hash__(), its instances will not be usable as items in hashable collections.

https://docs.python.org/3/reference/datamodel.html#object.__hash__

If there is not need to make _UnarySequenceOperator (and everything deriving it) hashable (@Dr15Jones ?), then perhaps adding an explicit check in _replaceIfHeldDirectly would be the way to.

[Dr15Jones]

If there is not need to make _UnarySequenceOperator (and everything deriving it) hashable (@Dr15Jones ?), then perhaps adding an explicit check in _replaceIfHeldDirectly would be the way to.

_UnarySequenceOperator may need to be hashable if things like cms.EDProducer and cms.EDFilter need to be hashable. This is becuase cms.ignore(cms.EDFilter(...)) could be used in a cms.Sequence and I don't know if items in a sequence are sometimes added to a hashed container.

[makortel]

that reseting aValue as a PSet removes the original requirement

Good point, thanks @davidlange6, so the attempted test is flawed. The test should really use some native type in the assignment instead of cms.Foo.

[Dr15Jones]

When playing around with the test, I tried lambda : setattr(p1, 'aValue', "foo") and that didn't raise the exception! Looks like python happily convert a string to an int? Doing lambda : setattr(p1, 'aValue', 1.3) did raise the exception.

[davidlange6]

String was an allowed type for aValue, no?

…

On Jul 14, 2021, at 3:51 PM, Chris Jones ***@***.***> wrote: When playing around with the test, I tried lambda : setattr(p1, 'aValue', "foo") and that didn't raise the exception! Looks like python happily convert a string to an int? Doing lambda : setattr(p1, 'aValue', 1.3) did raise the exception. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[Dr15Jones]

String was an allowed type for aValue, no?

Ooops, I missed that. Thanks.

[makortel]

This commit dd717c3 makes all of these tests work for me with python 3 in CMSSW_12_0_X_2021-07-12-2300. It does make _UnarySequenceOperator hashable.

[makortel]

@Dr15Jones found out a problem in the previous commit (see discussion there), here is a new one that resolves that issue: 737634e8118

[makortel]

Seems that @Dr15Jones and I have converged that 737634e8118 is ok. @smuzaffar Do you want to cherry-pick it here, or how should we proceed?

[smuzaffar]

@makortel , can you please open a new PR based on 737634e + python3 fix for FWCore/ParameterSet/test/runPythonTests.sh

[makortel]

Certainly

[makortel]

Done in #34493.

[smuzaffar]

closing in favor of #34493