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

Refactoring RunConfigAPI.configureRun() #4912

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Conversation

gkfthddk
Copy link
Contributor

@gkfthddk gkfthddk commented Feb 1, 2024

Replay Request

Requestor
Team or person that requests this replay

Describe the configuration

  • Release:
  • Run:
  • GTs:
    • expressGlobalTag:
    • promptrecoGlobalTag:
  • Additional changes:

Purpose of the test
A replay test is costly, both in computational and human resources. Please describe the reason why this test is needed.

T0 Operations cmsTalk thread
If necessary, provide a link to the cmsTalk thread announcing the test to the relevant groups.
Tier0 Operations cmsTalk Forum

@germanfgv
Copy link
Contributor

germanfgv commented Feb 5, 2024

@gkfthddk It looks good! We need to test it before we can merge it. Can you create some simply unit tests for the new functions you created?

They should be located here:
https://github.com/dmwm/T0/tree/master/test/python/T0_t/RunConfig_t

@gkfthddk
Copy link
Contributor Author

gkfthddk commented Feb 6, 2024

run unit test please

@cmsdmwmbot
Copy link

Container Tests
Unit tests finished

1 similar comment
@cmsdmwmbot
Copy link

Container Tests
Unit tests finished

@gkfthddk
Copy link
Contributor Author

run unit test please

@cmsdmwmbot
Copy link

Container Tests
Unit tests finished

@gkfthddk
Copy link
Contributor Author

I'm worried that the separation of functions and shortening of lines have made the script too arbitrary.

@gkfthddk
Copy link
Contributor Author

@germanfgv How can I run unit test? The unit test at jenkins fails.

@germanfgv
Copy link
Contributor

Please add your unit test in a different file, specific to RunConfigAPI. Put it in the same folder, with name RunConfigAPI_t.py

About running the test, you may find the information you need here:
https://cms-wmcore.docs.cern.ch/wmcore/setup-wmcore-unittest/#executing-wmcore-unittest

You can run it in one of our VMs

@germanfgv
Copy link
Contributor

@gkfthddk I fixed a conflict introduced by recent developments. Also, I added instructions to our doc on how to run Unit Tets
https://cmst0.docs.cern.ch/dev/t0agent/#running-unit-tests

Copy link
Contributor

@germanfgv germanfgv left a comment

Choose a reason for hiding this comment

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

I think if looks good and you came up with good ideas on how to reduce duplicate code. Please add docsctrings to the new functions you created. Focus on describing what the function does in the context of T0. I made a few suggestion of changes to islustrate this point.

Consider also to add or modify existing comments in the sections of code you have modified.

src/python/T0/RunConfig/RunConfigAPI.py Outdated Show resolved Hide resolved
src/python/T0/RunConfig/RunConfigAPI.py Outdated Show resolved Hide resolved
@germanfgv
Copy link
Contributor

I removed insertDAO mapping as the names in the mappings were wrong and I prefer having the individual DAOs for readability

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.

3 participants