[feat] Add custom commands for topology detection in remote partition #3281
Conversation
|
Hello @Blanca-Fuentes, Thank you for updating! Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide Comment last updated at 2024-10-10 11:25:37 UTC |
There was a problem hiding this comment.
For me this is a very good start. It would be good to have some unittests that check the basic functionality f the feature.
Some things that maybe we still need to discuss:
- Should we copy the reframe directory in the custom command case? I think not, as long as we don't have a use case for it.
- Do we give the option to the user to add commands pre/post in the current scripts (bootstrap/pip)?
| if use_pip: | ||
| _emit_script_for_pip(job, [part.local_env]) | ||
| else: | ||
| _emit_script_for_source(job, [part.local_env]) |
There was a problem hiding this comment.
| if use_pip: | |
| _emit_script_for_pip(job, [part.local_env]) | |
| else: | |
| _emit_script_for_source(job, [part.local_env]) | |
| elif use_pip: | |
| _emit_script_for_pip(job, [part.local_env]) | |
| else: | |
| _emit_script_for_source(job, [part.local_env]) |
No need to indent this further I think
| @@ -587,6 +591,7 @@ | |||
| "general/purge_environment": false, | |||
| "general/remote_detect": false, | |||
| "general/remote_workdir": ".", | |||
| "general/remote_command": [], | |||
There was a problem hiding this comment.
Keep alphabetical order in the list
| # Test that a dummy topo.json file is created if a command to do | ||
| # so is specified as custom command for remote detection |
There was a problem hiding this comment.
At the moment we are only triggering the autodetection, but cannot tell why the topo.json was created (since the reframe command will be added in the end). This is fine if it's not possible to check the script, but maybe update the comment to reflect that? 🤔
There was a problem hiding this comment.
With my suggestion of only requiring the reframe installation commands, this unit test would only need to check that the installation command was executed. We could for example have a dummy installation command touch foo.txt and here we could check that foo.txt exists.
There was a problem hiding this comment.
I like the idea of allowing users to specify the exact commands to run, but I would rather restrict that to the installation commands only. The reframe --detect-host-topology=topo.json should be added always by the framework.
As a result we should rather call the configuration variable as remote_install.
| if site_config.get('general/0/remote_detect') and \ | ||
| site_config.get('general/0/remote_command'): | ||
| remote_commands = site_config.get('general/0/remote_command') | ||
| detect_topology_cmd = "reframe --detect-host-topology=topo.json" | ||
| detect_topology_found = [ | ||
| s for s in remote_commands if detect_topology_cmd in s | ||
| ] | ||
| if not detect_topology_found: | ||
| remote_cmd_args = site_config.get('general/0/remote_command') | ||
| printer.debug( | ||
| "Adding ReFrame topology detection to the curstom command" | ||
| ) | ||
| remote_cmd_args.append(detect_topology_cmd) |
There was a problem hiding this comment.
This is not needed. We should always add the reframe command to execute.
There was a problem hiding this comment.
What if the command needs to be ./bin/reframe ..., like it is the case for the bootstrap option?
There was a problem hiding this comment.
I guess that would be the case when sb wants to clone explicitly and bootstrap it, because by default we make a copy (NB: the method we discuss is something extra to what we do now). Well, in this case the user could set the PATH to the local bin/:
remote_install = ['git clone ... && cd reframe && ./boostrap.sh && cd -',
'export PATH=$PWD/reframe/bin:$PATH']| @pytest.fixture | ||
| def remote_custom_exec_ctx(make_exec_ctx, temp_topo): | ||
| if test_util.USER_CONFIG_FILE is None: | ||
| pytest.skip('no user configuration file supplied') | ||
|
|
||
| ctx = make_exec_ctx(test_util.USER_CONFIG_FILE, | ||
| test_util.USER_SYSTEM, | ||
| {'general/remote_detect': True, | ||
| 'general/remote_command': [ | ||
| 'echo \'{"dummy": "value"}\' > topo.json', | ||
| ]}) | ||
| yield ctx |
There was a problem hiding this comment.
Just extend the existing remote_exec_ctx to accept additional configuration options.
| # Test that a dummy topo.json file is created if a command to do | ||
| # so is specified as custom command for remote detection |
There was a problem hiding this comment.
With my suggestion of only requiring the reframe installation commands, this unit test would only need to check that the installation command was executed. We could for example have a dummy installation command touch foo.txt and here we could check that foo.txt exists.
This PR adds the option of specifying custom commands to run
reframe --detect-host-topology=topo.jsonon a remote partition to detect the topology.Previously, a clone of reframe was created in order to launch reframe with auto-detect option.
Now, a list of custom commands can be specified through the configuration option
general:remote_commandto avoid creating a reframe clone withpipor running.\bootstrap.sh.The
reframe --detect-host-topology=topo.jsonmust be specified in the list of commands. If it is not, it is added at the end.Closes #2292.
Closes #2690.
Closes #2979 .