-
Notifications
You must be signed in to change notification settings - Fork 789
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
Use the new version for the McM REST client #3750
Conversation
1. Remove all the instructions that load this module from AFS. 2. Raise a detailed exception explaining how to install the new version if the module is not available. 3. Update instructions that used `mangled` methods available in the McM class to avoid `DeprecationWarning` messages
Hi @efeyazgan. These changes are related to using the new version for the McM REST client and avoiding importing modules from AFS. I saw there are some CI jobs to check changes for the script. However, they do not check the contribution provided in the PR (the commits differ - PR: Thanks, |
Hi @ggonzr
This happens because of
Is this new method to be used in the same way in lxplus (locally) and elsewhere? Then, I can drop that if. However, even when I drop that statement I get the same error. Before running this command, I do
Am I missing anything? |
Yes, you need to install the package by executing this pip instruction. By the way, is this script meant to be used only by the McM application, or do other users/applications use it? |
OK, I'll try and let you know. This is being used both by McM and many users locally (in lxplus). |
OK, after installing the package, it worked. I think it is OK to merge. |
Dear all, One point before we merge, this script is run on condor (unless you changed this recently), so installation should be also implemented in the script itself, no? Thanks |
Is there a script running the request checking script on condor? If so, that should be updated. |
I think it is the get test that is executed on condor. (IIRC, current PdmV and Geovanny can correct me if wrong). So indeed it can be added on the line before the script is run. Thanks |
Indeed, this script is embedded to run McM validation jobs in HTCondor and prepare other kinds of scripts that the application provides. But as described before, #1138 takes care of this. The only remaining thing would be to send an announcement to other users about this change, which is a breaking change in case they run the script directly in their workflows as they have to also prepare an environment that installs the package. Do you know if this situation happens often and who could be affected by this? Perhaps, instead of raising an error, we could try to temporarily import the package available in AFS and raise a |
Hi Geovanny
Thankw for clarification, then indeed i would add a warning and a deadline, then we can switch to error.
Cheers
B
Envoyé de mon iPhone
Le 30 août 2024 à 09:57, Geovanny González-Rodríguez ***@***.***> a écrit :
Indeed, this script is embedded to run McM validation jobs in HTCondor and prepare other kinds of scripts that the application provides. But as described before, #1138<cms-PdmV/cmsPdmV#1138> takes care of this.
The only remaining thing would be to send an announcement to other users about this change, which is a breaking change in case they run the script directly in their workflows as they have to also prepare an environment that installs the package. Do you know if this situation happens often and who could be affected by this?
Perhaps, instead of raising an error, we could try to temporarily import the package available in AFS and raise a DeprecationWarning alerting on this change giving users a window of 3 weeks to 1 month to update their environments before we cut the support on this old version. Do you agree with this approach?
—
Reply to this email directly, view it on GitHub<#3750 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABQ4O2VE2B7JMIN62RXHWJDZUAQXDAVCNFSM6AAAAABMZ3FYBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRQGQYDGMZZGQ>.
You are receiving this because you commented.Message ID: ***@***.***>
|
That seems OK (for the ones who want to run locally, I summarized the commands here: https://github.com/cms-sw/genproductions/blob/master/bin/utils/run_req_new.sh) |
This is intended for a smoother integration to avoid import errors with the user's code. This is a temporary workaround that will be removed in the future. 1. Re-use old function signatures accessing internal methods for the `McM` object. 2. Import the old version from AFS if required.
Hi @efeyazgan, |
I just rechecked this with the integration for McM described in #1138 and it works by providing a |
Sorry. I had missed this. I tested it and it works OK. Thanks a lot! |
Thanks! Looks good to me, I only have one question to make sure I understand. I think MC contacts usually download the test script from McM (using the "Get test command") to run some quick validation locally on lxplus, before running the McM validation. Can you please confirm that this will still work, provided you merge also cms-PdmV/cmsPdmV#1138? |
Yes, it will work even if cms-PdmV/cmsPdmV#1138 is not merged immediately as the script will load the old version without issues if it is executed in Lxplus nodes. So it is safe to merge this |
Related to: #23, #1138
Use the new version for the McM REST client
mangled
methods available in the McM class to avoidDeprecationWarning
messages.