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

Alter launchers to pass env when starting a local step #329

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Jul 26, 2023

This ticket ensures that an experiment with a workload manager will pass environment variables to unmanaged steps. This edge case occurs if a developer manually creates and attach a RunSettings instance instead of using the experiment.create_run_settings factory method.

Changes include:

  • minor formatting (multi-line statements converted to single line)
  • LocalStep environment is passed to task manager when task is unmanaged by launcher
  • slurm-specific tests added to verify task run in popen inherits calling env

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #329 (15b8d9f) into develop (4c741be) will decrease coverage by 0.13%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #329      +/-   ##
===========================================
- Coverage    87.30%   87.18%   -0.13%     
===========================================
  Files           59       59              
  Lines         3522     3518       -4     
===========================================
- Hits          3075     3067       -8     
- Misses         447      451       +4     
Files Changed Coverage Δ
smartsim/_core/launcher/local/local.py 94.00% <ø> (-0.45%) ⬇️
smartsim/_core/launcher/taskManager.py 77.24% <ø> (-2.40%) ⬇️

@ankona ankona marked this pull request as ready for review July 26, 2023 20:35
@ankona ankona marked this pull request as draft July 26, 2023 20:35
@mellis13 mellis13 changed the title alter launchers to pass env when starting a local step Alter launchers to pass env when starting a local step Jul 27, 2023
@ankona ankona marked this pull request as ready for review July 27, 2023 19:54
Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Small changes to ensure that the test doesn't fail on non-slurm machines. Other question, do you want to reserve adding tests for the other WLMs for later? It's a little challenging because of the combinations of WLMs and run commands

Thanks for digging into this one!

tests/on_wlm/test_local_step.py Show resolved Hide resolved
@ankona ankona requested a review from MattToast July 27, 2023 21:24
@ankona ankona requested a review from ashao July 28, 2023 15:10
Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Let's hold off on adding PBS+aprun tests since it might be a little harder to detect the availability of aprun.

@ankona ankona merged commit 9d7ac35 into CrayLabs:develop Jul 28, 2023
10 checks passed
@MattToast MattToast added the area: launcher Issues related to any of the launchers within SmartSim label Sep 13, 2023
@ankona ankona deleted the oss476-2 branch April 4, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: launcher Issues related to any of the launchers within SmartSim
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants