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 change dbs dataset file status fix #5204 #5241

Merged
merged 15 commits into from
Oct 27, 2023
Merged

Add change dbs dataset file status fix #5204 #5241

merged 15 commits into from
Oct 27, 2023

Conversation

belforte
Copy link
Member

@belforte belforte commented Oct 25, 2023 •
edited
Loading

I think this is OK now. Aside from

  • setfilestatus does not work with a list of files. Valentin acknowledged a bug but it is for others to fix. We can live w/o it for quite some time
  • tune/polish help (remove single char commands ? now that we have autocompletion...)
  • whatever @novicecpp and @mapellidario suggest

@belforte
Copy link
Member Author

belforte commented Oct 25, 2023 •
edited
Loading

I did a quick test on CMSSW_9 on lxplus7 (python2) and it is OK. Once have test in Jenkins will run all archs
and of course I need to make sure I did not break anything !

@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@belforte belforte mentioned this pull request Oct 25, 2023
Closed
5 tasks
novicecpp
novicecpp requested changes Oct 26, 2023
View reviewed changes
src/python/CRABClient/Commands/setdatasetstatus.py Outdated Show resolved Hide resolved
src/python/CRABClient/Commands/setfilestatus.py Show resolved Hide resolved
src/python/CRABClient/Commands/setdatasetstatus.py Outdated Show resolved Hide resolved
src/python/CRABClient/CrabRestInterface.py Outdated Show resolved Hide resolved
@belforte
Copy link
Member Author

thanks a lot @novicecpp
I have a question for you now: would it then be better to also move HTTPRequests to ClientUtilities ? Since it is not used by CRABRest only.

@belforte
Copy link
Member Author

I merged my branch with master dmwm repo in test-setdatasetfilesstatus branch tagged as testSetDsetFilesStatus.1 and ran Jenkins CV to check for regression

All OK !

@belforte
Copy link
Member Author

@novicecpp I captured your comments before in this list of actions. I will add more as needed once you answer my questions. Feel free to check and comment in there. Thanks

@novicecpp
Copy link
Contributor

I have a question for you now: would it then be better to also move HTTPRequests to ClientUtilities ? Since it is not used by CRABRest only.

Maybe move HTTPRequests to utils and move DBSREST to new file same level as CrabRestInterface.py?

@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@belforte
Copy link
Member Author

belforte commented Oct 26, 2023 •
edited
Loading

Hope I addressed all coments (only the uri vs. api confusion is left, but I think it is too much work). Will run Jenkins again.
New code tagged as testSetDsetFilesStatus.2
Jenkinks OK

@cmsdmwmbot

This comment was marked as outdated.

Sign in to view
@belforte
Copy link
Member Author

code with last changes is now tagged as testSetDsetFilesStatus.2 in the ad-hoc test branch

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Pylint check: failed
    • 23 warnings and errors that must be fixed
    • 9 warnings
    • 246 comments to review
  • Pycodestyle check: succeeded
    • 666 comments to review

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

@belforte
Copy link
Member Author

ooopppss. I had added content-type in the wrong place. But of course CRAB Server REST happily ignores it and everything kept working, but better not confuse the reader.

@belforte
Copy link
Member Author

If there are no more objection I will do a BIG SQUASH and merge.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Pylint check: failed
    • 23 warnings and errors that must be fixed
    • 9 warnings
    • 246 comments to review
  • Pycodestyle check: succeeded
    • 666 comments to review

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

novicecpp
novicecpp approved these changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@novicecpp novicecpp left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks.

@belforte
Copy link
Member Author

thanks @mapellidario and @novicecpp for review and advice. I will merge. And we can discuss separately if/how to add --recursive. Maybe wait for someone to ask for it 😁 ?

We still have time to decide if to keep old semantic (Invalidating a dataset will automatically invalidate all the files in the dataset) or leave it to user with two actions as I indicated above. It seems a soft decisions, with arguments in pro/con either choice.

@belforte
Copy link
Member Author

Since in current implementation crab setdatasetstatus does not change status of individual files, I will print a note about this when the command is executed !

@mapellidario
Copy link
Member

Thanks Stefano for the detailed description of dataset and file validity in DBS. Sorry for the dumb questions!

Do we already have an entry in the FAQs about DBS dataset and file status? Since we are allowing users to change it, we should also provide clear instructions about what those are and what the commands we provide do (or plan to do).

@belforte
Copy link
Member Author

Dario,your questions were not dumb. The problem is that there is no real rule on how to use those status flags,
DBS allows to set, then everybody can do as they see fit. I think Production changed their practice over time.

As user documentation this is what we have:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/CRAB3FAQ#Can_I_delete_a_dataset_I_publish
it clearly needs a rewriting ! That is my next step.

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Pylint check: failed
    • 23 warnings and errors that must be fixed
    • 9 warnings
    • 246 comments to review
  • Pycodestyle check: succeeded
    • 666 comments to review

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

@belforte belforte force-pushed the add-change-DBS-dataset-file-status-fix-5204 branch from 4e5f421 to 348d546 Compare October 27, 2023 19:34
@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Pylint check: failed
    • 23 warnings and errors that must be fixed
    • 9 warnings
    • 246 comments to review
  • Pycodestyle check: succeeded
    • 666 comments to review

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

@belforte belforte merged commit 33afb42 into dmwm:master Oct 27, 2023
@belforte belforte deleted the add-change-DBS-dataset-file-status-fix-5204 branch October 27, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mapellidario mapellidario mapellidario requested changes

@novicecpp novicecpp novicecpp approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@belforte @cmsdmwmbot @novicecpp @mapellidario