Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[py2py3] Migration at level scr/python/A/B/C - slice 17 #10329

Merged
merged 2 commits into from
Mar 12, 2021

Conversation

mapellidario
Copy link
Member

@mapellidario mapellidario commented Mar 4, 2021

Fixes #10141

Status

Ready

Related PRs

Description

Run futurize and some manual changes on the first batch of src/python/A/B/C.

"native string" approach

  • src/python/WMCore/FwkJobReport/Report.py: imported str from builtins, but used only in a narrow case.
  • src/python/WMCore/WMRuntime/Scripts/SetupCMSSWPset.p: there is only one line where this may be problematic: inputTypeAttrib.fileNames.append(str(fileLFN)), but I do not think that there is any incentive to try to change it at this stage. tracked at [py2py3] test str/bytes use in WMCore (wildcard issue) #10323

Is it backward compatible (if not, which system it affects?)

It should be. (Any possible cause for errors will we reported here)

External dependencies / deployment changes

Requires python-future in both py2 and py3 environments.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

except UnicodeEncodeError as ex:
msg = "Failed to decode the job error details for job ID: %s." % self.getJobID()
# if the `try` fails in py3, then the following line will also
# fail in python3 and there is not much that we can do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alan, we can remove this comment if you think it is irrelevant!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because it will fail to stringify errorDetails in the error message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! that is correct! But since you rightfully pointed out that we can have malformed input, maybe I should completely change the meaning of this try/catch. I am working on it!

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot
Copy link

Jenkins results:

  • Unit tests: succeeded
    • 1 changes in unstable tests
  • Pylint check: failed
    • 51 warnings and errors that must be fixed
    • 18 warnings
    • 252 comments to review
  • Pylint py3k check: succeeded
    • 0 errors and warnings that should be fixed
    • 0 warnings
    • 0 comments to review
  • Pycodestyle check: succeeded
    • 143 comments to review
  • Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11431/artifact/artifacts/PullRequestReport.html

@mapellidario
Copy link
Member Author

@amaltaro you are the one that should make the last call! I replaced replace with ignore!
I pushed two new commits that contain all the changes after your first review, these are ready to be review as well! Thanks again for the feedback!

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot

This comment has been minimized.

@cmsdmwmbot
Copy link

Jenkins results:

  • Unit tests: succeeded
  • Pylint check: failed
    • 52 warnings and errors that must be fixed
    • 18 warnings
    • 259 comments to review
  • Pylint py3k check: failed
    • 0 errors and warnings that should be fixed
    • 1 warnings
    • 0 comments to review
  • Pycodestyle check: succeeded
    • 153 comments to review
  • Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11470/artifact/artifacts/PullRequestReport.html

@amaltaro
Copy link
Contributor

Thanks Dario. Last 2 commits look good to me as well. Can you please squash them accordingly? Thanks

@cmsdmwmbot
Copy link

Jenkins results:

  • Unit tests: succeeded
  • Pylint check: failed
    • 52 warnings and errors that must be fixed
    • 18 warnings
    • 259 comments to review
  • Pylint py3k check: failed
    • 0 errors and warnings that should be fixed
    • 1 warnings
    • 0 comments to review
  • Pycodestyle check: succeeded
    • 153 comments to review
  • Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11474/artifact/artifacts/PullRequestReport.html

@amaltaro
Copy link
Contributor

Thanks Dario.

@amaltaro amaltaro merged commit b50e6e3 into dmwm:master Mar 12, 2021
mapellidario added a commit to mapellidario/WMCore that referenced this pull request Jun 17, 2021
mapellidario added a commit to mapellidario/WMCore that referenced this pull request Jun 17, 2021
mapellidario added a commit to mapellidario/WMCore that referenced this pull request Jun 17, 2021
mapellidario added a commit to mapellidario/WMCore that referenced this pull request Jun 17, 2021
mapellidario added a commit to mapellidario/WMCore that referenced this pull request Jun 17, 2021
mapellidario added a commit to mapellidario/WMCore that referenced this pull request Jun 17, 2021
mapellidario added a commit to mapellidario/WMCore that referenced this pull request Jun 17, 2021
@mapellidario mapellidario deleted the 17-10141-py2py3 branch March 10, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py2py3 slice Issue related to slicing the modernization to py2py3 Python3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[py2py3] Migration at level scr/python/A/B/C - slice 17
3 participants