Skip to content

[v7r3] move Script goods to DIRACScript#5035

Merged
fstagni merged 43 commits into
DIRACGrid:integrationfrom
TaykYoku:v7r2_docs_fix_2
Aug 3, 2021
Merged

[v7r3] move Script goods to DIRACScript#5035
fstagni merged 43 commits into
DIRACGrid:integrationfrom
TaykYoku:v7r2_docs_fix_2

Conversation

@TaykYoku

@TaykYoku TaykYoku commented Mar 14, 2021

Copy link
Copy Markdown
Contributor

UPDATE:
additional testing did not reveal similar problems described here, so this reason I think is leveled

This PR is designed to solve the problem described in comment(a more global solution then #5036). Avoiding the use of global variables in favor of instance variables improves the situation.

UPDATE:

  1. IIUC that this PR will go to v7r3 as well as [v7r2 --> v7r3(#5035)] register script arguments #5024. Taking this into account, and the fact that these PRs are designed to modify the same files, I decided to combine them here to make reviewing easier.
  2. the use of [v7r2 --> v7r3(#5035)] register script arguments #5024 features is optional, but since changes to DIRACScript made before merging were required to edit each affected DIRACScript script, so I have added argument registration for "full testing".
  3. the changes should not affect the expected result of the scripts running.

More about PR, since DIRACScript will be used every time the command is called, the main change concerns this class, namely the transfer of logic from Script as instance variables/methods.

Added the ability to register positional arguments and add it to help message from #5024.
Note: Applied

Places where argument registration was not applied:

  • src/DIRAC/DataManagementSystem/scripts/dirac_dms_add_file.py
  • src/DIRAC/FrameworkSystem/scripts/dirac_sys_sendmail.py
  • src/DIRAC/StorageManagementSystem/scripts/dirac_stager_stage_files.py
  • src/DIRAC/TransformationSystem/scripts/dirac_production_runjoblocal.py
  • tests/Jenkins/dirac-... .py

Main changes:

  • DIRACScript:
    • added general methods and initialization part from Script
    • in the DIRACScripts search cycle among the entry_points, a filter by scriptName is added, @chrisburr could you review this change, please?

UPDATE: This already done with #5061

  • LocalConfiguration:
    • added registerCmdArg to register arguments
    • added group parameter to getPositionalArguments to to get grouped arguments as they were registered
    • added test

Changes for the scripts(e.g.: dirac-my-great-script):

  • Use new method: registerArgument
  • remove Script import
from DIRAC.Core.Base import Script
from DIRAC.Core.Utilities.DIRACScript import DIRACScript

a = None

def setA(arg):
  global a
  a = arg

@DIRACScript()
def main():
  global a
  Script.registerSwitch(..., setA)

to

from DIRAC.Core.Utilities.DIRACScript import DIRACScript as Script

a = None

def setA(arg):
  global a
  self.a = arg

@Script()
def main():
   Script.registerSwitch(..., setA)
   Script.registerArgument("my arg: my argument")

BEGINRELEASENOTES

*allSystems/scripts
CHANGE: use registerArgument to register positional arguments for scripts
FIX: use DIRACScript instead Script

ENDRELEASENOTES

@TaykYoku TaykYoku changed the title docs fix [v7r2] move Script goods to DIRACScript Mar 15, 2021
@chrisburr

Copy link
Copy Markdown
Member

@TaykYoku This looks like a really nice change to remove some of the global state. Can you add an explanation of what is being changed to the PR description to make this easier to review?

Also given how large the changes are and how poorly tested the scripts are in the CI I think this might be better suited to v7r3. If others agree I can come up with a hack to workaround the problem mentioned in #5032 (comment).

@TaykYoku

Copy link
Copy Markdown
Contributor Author

@chrisburr agree that many fixes delay the release.
I offer #5036 to bypass the problem, if you haven't already.

@fstagni fstagni changed the title [v7r2] move Script goods to DIRACScript [v7r3] move Script goods to DIRACScript Mar 16, 2021

@fstagni fstagni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are some scripts also inside the /tests directory

Comment thread tests/Jenkins/utilities.sh Outdated
@TaykYoku

Copy link
Copy Markdown
Contributor Author

given that this PR will go to v7r3 as well as #5024, then I can combine them here, to avoid duplication of work on their review. They both affect the same files

@TaykYoku TaykYoku force-pushed the v7r2_docs_fix_2 branch 2 times, most recently from f69c661 to e4251ee Compare March 17, 2021 12:23
@chrisburr

Copy link
Copy Markdown
Member

Perhaps it would be a good idea to discuss how you propose to change the scripts before doing the tedious work of converting them all?

@TaykYoku

TaykYoku commented Mar 17, 2021

Copy link
Copy Markdown
Contributor Author

I wanted to protest whether it would work in all cases, but I think you are right.
How best to open a discussion about this?

P.S.: now I'm not improving anything, I'm just fixing bugs to pass the tests(this will simplify reviewing)

@TaykYoku TaykYoku changed the title [v7r3] move Script goods to DIRACScript [v7r3] move Script goods to DIRACScript(wait #5061) Apr 2, 2021
@TaykYoku TaykYoku marked this pull request as draft April 11, 2021 22:32
@TaykYoku TaykYoku closed this Jul 11, 2021
@TaykYoku

Copy link
Copy Markdown
Contributor Author

restart tests

@TaykYoku TaykYoku reopened this Jul 11, 2021
@TaykYoku TaykYoku closed this Jul 12, 2021
@TaykYoku

Copy link
Copy Markdown
Contributor Author

retest

@TaykYoku TaykYoku reopened this Jul 12, 2021
@TaykYoku

TaykYoku commented Jul 12, 2021

Copy link
Copy Markdown
Contributor Author

since the main issue was solved by a separate PR, I rebase/simplified this PR by leaving the transfer of goods from Script and registering positional arguments to display them in the help message.

@TaykYoku TaykYoku marked this pull request as ready for review July 12, 2021 11:32
Comment thread src/DIRAC/DataManagementSystem/scripts/dirac_dms_change_replica_status.py Outdated
Comment thread src/DIRAC/DataManagementSystem/scripts/dirac_dms_move_replica_request.py Outdated
Comment thread src/DIRAC/Interfaces/scripts/dirac_admin_reset_job.py Outdated
TaykYoku and others added 3 commits July 19, 2021 14:22
…a_status.py

Co-authored-by: fstagni <federico.stagni@cern.ch>
…request.py

Co-authored-by: fstagni <federico.stagni@cern.ch>
Co-authored-by: fstagni <federico.stagni@cern.ch>
@fstagni fstagni linked an issue Jul 20, 2021 that may be closed by this pull request
@fstagni fstagni merged commit 67d0389 into DIRACGrid:integration Aug 3, 2021
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.

Command line script changes

3 participants