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.

Sign up for GitHub

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

Jump to bottom

Add campaign name attribute to base WMTask object and propagate it as a classad #11710

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Add campaign name attribute to base WMTask object and propagate it as a classad #11710

merged 2 commits into from
Sep 19, 2023

Conversation

khurtado
Copy link
Contributor

@khurtado khurtado commented Sep 8, 2023 •
edited by amaltaro

Fixes #11704
Fixes #11705

Status

In development

Description

Add campaign name attribute to base WMTask object and propagate it as a classad. This should work for RERECO, not child tasks taken into account.

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

YES

Related PRs

None

External dependencies / deployment changes

None

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 24 warnings and errors that must be fixed
    • 11 warnings
    • 269 comments to review
  • Pylint py3k check: failed
    • 1 warnings
  • Pycodestyle check: succeeded
    • 23 comments to review

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

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 1 tests no longer failing
    • 3 changes in unstable tests
  • Python3 Pylint check: failed
    • 25 warnings and errors that must be fixed
    • 19 warnings
    • 297 comments to review
  • Pylint py3k check: failed
    • 1 warnings
  • Pycodestyle check: succeeded
    • 26 comments to review

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

@khurtado
Copy link
Contributor Author

khurtado commented Sep 8, 2023

@amaltaro Unit tests are still missing, but this was the general idea for RERECO workflows, right?

@amaltaro
Copy link
Contributor

amaltaro commented Sep 8, 2023

@khurtado Yes Kenyi. I think you covered it all and perhaps it even addresses the TaskChain case too.
As you go through the unit tests, I would suggest you to look into the TaskChain unit tests as well and expand one of them to test if this PR covers the TaskChain needs as well. Thanks

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 2 tests added
  • Python3 Pylint check: failed
    • 38 warnings and errors that must be fixed
    • 19 warnings
    • 357 comments to review
  • Pylint py3k check: failed
    • 1 warnings
  • Pycodestyle check: succeeded
    • 41 comments to review

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

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 4 tests added
  • Python3 Pylint check: failed
    • 59 warnings and errors that must be fixed
    • 19 warnings
    • 489 comments to review
  • Pylint py3k check: failed
    • 1 warnings
  • Pycodestyle check: succeeded
    • 121 comments to review

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

@khurtado
Copy link
Contributor Author

test this please

@khurtado
Copy link
Contributor Author

@amaltaro I added the unit tests. I still need to test this in my private services though

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 1 new failures
    • 4 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: failed
    • 59 warnings and errors that must be fixed
    • 19 warnings
    • 489 comments to review
  • Pylint py3k check: failed
    • 1 warnings
  • Pycodestyle check: succeeded
    • 121 comments to review

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

@khurtado
Copy link
Contributor Author

khurtado commented Sep 15, 2023 •
edited

@amaltaro If we have e.g.: A LogCollect job, we currently have campaignName = None . Should we simply not create a CMS_CampaignName condor classad for that job? In other words, should we propagate CMS_CampaignName only for cmssw jobs?

@amaltaro
Copy link
Contributor

I am inclined to say that we should provide it as a classad, even if is None/undefined for such utilitarian jobs.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 3 tests no longer failing
    • 4 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: failed
    • 59 warnings and errors that must be fixed
    • 19 warnings
    • 490 comments to review
  • Pylint py3k check: failed
    • 1 warnings
  • Pycodestyle check: succeeded
    • 121 comments to review

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

@khurtado
Copy link
Contributor Author

@amaltaro Okay, I added an extra line to set it as undefined in those cases. Code was tested and is ready for a review.

amaltaro
amaltaro requested changes Sep 18, 2023
View reviewed changes
Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

@khurtado Kenyi, I left some minor comments along the code for your consideration, but I think it would be great to get them addressed.

In addition to that:

  • please squash the commits accordingly (don't forget to leave test/* changes in a separate commit please)
  • the initial PR description needs to be updated (for the template placeholders, if you have nothing to replace that with, then just use None please)
  • I see you provide TaskChain unit tests as well. Does it mean this PR is supposed to fix Support campaign name for TaskChain workflows #11705 as well (I feel like it should)?

src/python/WMCore/BossAir/Plugins/SimpleCondorPlugin.py Outdated Show resolved Hide resolved
src/python/WMCore/WMSpec/WMTask.py Show resolved Hide resolved
src/python/WMCore/WMSpec/WMTask.py Outdated Show resolved Hide resolved
test/python/WMComponent_t/JobCreator_t/JobCreator_t.py Outdated Show resolved Hide resolved
Kenyi Hurtado Anampa added 2 commits September 18, 2023 16:56
Add campaign name attribute to base WMTask object and propagate it to…
2060625 
… the job level as a classad

Do not get campaign name from workload object but the task in the job object

Set CampaignName as undefined when its value is None
Add unit tests.
476f016
@khurtado
Copy link
Contributor Author

khurtado commented Sep 18, 2023 •
edited

@amaltaro I just addressed you comments. And yes, this seems to fix #11705 as well. I just wanted to finish running a TC workflow before commenting on it on that issue, but my MS-Transferor service is having authentication issues right now.

I added a "Do not merge yet" label just to double check on that test when I can get it done. Then, this could be merged and both issues would be addressed.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 2 new failures
    • 4 tests no longer failing
    • 4 tests added
  • Python3 Pylint check: failed
    • 59 warnings and errors that must be fixed
    • 19 warnings
    • 489 comments to review
  • Pylint py3k check: failed
    • 1 warnings
  • Pycodestyle check: succeeded
    • 121 comments to review

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

@khurtado khurtado mentioned this pull request Sep 19, 2023
Closed
@khurtado
Copy link
Contributor Author

@amaltaro This has been tested again with a teschain workflow and it worked. I have updated #11705. Once this PR is merged, we can close both issues

@amaltaro
Copy link
Contributor

Perfect, thanks Kenyi.
You missed the PR description update, so I went ahead and updated it myself.

@amaltaro amaltaro merged commit 25ee38f into dmwm:master Sep 19, 2023
1 of 4 checks passed
@amaltaro amaltaro mentioned this pull request Sep 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaltaro amaltaro amaltaro requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support campaign name for TaskChain workflows Support campaign name for RERECO workflows
3 participants
@khurtado @cmsdmwmbot @amaltaro