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

Copy environment #163

Merged
merged 4 commits into from
Dec 17, 2019
Merged

Copy environment #163

merged 4 commits into from
Dec 17, 2019

Conversation

MorganRodgers
Copy link
Contributor

Fixes #158.

Morgan Rodgers added 3 commits December 9, 2019 13:54
…vior to continue

- Add back missing Slurm environment setting
- Implement environment persistence for LSF, PBSPro, Torque and Grid Engine

I also noticed that multiple adapters when dealing with the contents of Script#job_environment explicitly stringify both the keys and the values; this is redundent as that mutation is performed in the Script constructor. Job submission always requires a Script object, which are immutable, and so the adapter-specific stringification sections in the code are safe to remove.
@johrstrom
Copy link
Contributor

Do we also need to populate the environment? That is, we call some open3 api with an environment hash, in lsf it's env = script.job_environment || {} that ends up being the actual env being passed into open3.capture3.

So my question is, if someone sets this flag is it sufficient to just populate the command, or do we actually need to env variables to this hash so they get passed into open3.capture3's env?

@MorganRodgers
Copy link
Contributor Author

I believe in every case the environment is being passed to the call implementation which uses open3.capture3.

@johrstrom
Copy link
Contributor

I think I found the answer from the open3 and process docs, I think that env is additive to the current ENV, not an out right replacement, so I think I answered my own question in that we only need to modify args unless there's somehting specific to env we need to deal with like in the case of slurm.

@ericfranz
Copy link
Contributor

Are you aware of the ramifications of having both -v and -V appear as arguments to qsub in SGE and PBSPro?

@MorganRodgers
Copy link
Contributor Author

MorganRodgers commented Dec 10, 2019

I am not; I have not yet tested them on live systems yet. That's one of the reasons why I am updating ood-images right now. Are you aware of any negative interactions between those flags / options?

@ericfranz
Copy link
Contributor

No I am not.

@MorganRodgers
Copy link
Contributor Author

I am going to test GridEngine and PBSPro tomorrow once I work through some Vagrant issues.

@MorganRodgers
Copy link
Contributor Author

GridEngine and PBSPro both behave as I would expect.

# Given a job.sh that echos its environment
A_ENV=value qsub job.sh                            # Does not output A_ENV
A_ENV=value qsub -V job.sh                         # Does output A_ENV
A_ENV=value qsub -v B_ENV=something_else job.sh    # Outputs B_ENV, but not A_ENV
A_ENV=value qsub -V -v B_ENV=something_else job.sh # Outputs A_ENV and B_ENV

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.

Slurm adapter: srun cannot see $PATH/other env vars when submitted through job composer
3 participants