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

Create job classad for REQUIRED_ARCH #11298

Merged
merged 2 commits into from
Oct 3, 2022
Merged

Conversation

amaltaro
Copy link
Contributor

@amaltaro amaltaro commented Sep 27, 2022

Fixes #11291

Status

ready

Description

Changes in this PR are:

  • created a new job classad for REQUIRED_ARCH, which is a comma separated string with the architectures supported by a given job
  • matchmaking logic remains exactly the same, but now it uses the freshly created new ad (REQUIRED_ARCH)
  • lastly, convert this module to python3-only (2nd commit)

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

YES

Related PRs

None

External dependencies / deployment changes

None

@amaltaro
Copy link
Contributor Author

@mmascher I still have to test it, but let me know if you have any comments on this.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 2 tests no longer failing
    • 2 changes in unstable tests
  • Python3 Pylint check: failed
    • 2 warnings and errors that must be fixed
    • 4 warnings
    • 47 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

@amaltaro
Copy link
Contributor Author

test this please

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 2 tests no longer failing
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 2 warnings and errors that must be fixed
    • 4 warnings
    • 47 comments to review
  • Pylint py3k check: succeeded
  • Pycodestyle check: succeeded
    • 13 comments to review

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

@amaltaro
Copy link
Contributor Author

This has been tested with standard workflows and it's working fine.

Copy link
Contributor

@todor-ivanov todor-ivanov left a comment

Choose a reason for hiding this comment

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

Thanks @amaltaro

The classadd related changes look good to me. But the python3 only modification being present in the very same PR do obfuscate the real change and makes it hard to spot. And provided they are indeed needed I'd suggest to separate them in a dedicated issue.

@amaltaro
Copy link
Contributor Author

Thanks @todor-ivanov . I have a slightly different view here, and I would say that if changes are small and safe enough, we should simply go ahead and start converting modules that we touch to python3-only. It's not a requirement, but welcome to make maintenance of the code easier.

In other words, we can deal with this python2/future code removal the same as pylint corrections/improvements. Of course, always keeping it in its own commit (and noting it in the PR description).

@amaltaro amaltaro merged commit 2d3f8d8 into dmwm:master Oct 3, 2022
@mmascher
Copy link
Member

mmascher commented Oct 4, 2022

Hi I just saw the changes @amaltaro . Looks good to me. Only one comment. IIRC, at some point Jaime recommended to add something like:

(TARGET.Arch =!= Undefined)

because HTCondor automatically adds (TARGET.Arch =!= 'XXX') (where XXX is the architecture of the scheduler) if there is no such requirement in the job classad. I assume this will still work even if we are using a classad like REQUIRED_ARCH, but it's worth keeping an eye for Marconi100 tests.

Also, a timely change! We just got an ARM machine from CERN, we are going to add it to the GWMS factory to do some tests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a new condor classad for required architecture
4 participants