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

flux-jobspec: Add environment to jobspec, support srun --export option #2227

Merged
merged 3 commits into from Jul 12, 2019

Conversation

@grondo
Copy link
Contributor

commented Jul 12, 2019

Add support in flux-jobspec for adding the current environment to generated jobspec by default. In addition, the srun --export option is supported in flux jobspec srun.

e.g.

  • flux jobspec srun --export=FOO: only export variable FOO from current env
  • flux jobspec srun --export=FOO=bar: explicitly export FOO=bar
  • flux jobspec srun --export=ALL,FOO=bar: export all current vars and add FOO=bar
  • flux jobspec srun --export=NONE: export empty environment

I think I got exception handling for flux jobspec srun --export=NONEXISTENT working correctly. Without the try,except block, I got a generic error like

$ src/cmd/flux jobspec srun --export=SOUP hostname 
util: ERROR: 'SOUP'

Maybe something to look into there regarding generic exception handling in the util package.

Fixes #2188

@grondo grondo requested a review from SteVwonder Jul 12, 2019
@grondo grondo changed the title flux-jobspec: Add current env to jobspec and srun --export option flux-jobspec: Add environment to jobspec, support srun --export option Jul 12, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Without the try,except block, I got a generic error like

Ah, one problem is likely this:

diff --git a/src/bindings/python/flux/util.py b/src/bindings/python/flux/util.py
index 9989eded4..081687ffa 100644
--- a/src/bindings/python/flux/util.py
+++ b/src/bindings/python/flux/util.py
@@ -73,7 +73,7 @@ class CLIMain(object):
 
     def __call__(self, main_func):
         logging.basicConfig(
-            level=logging.INFO, format="%(module)s: %(levelname)s: %(message)s"
+            level=logging.INFO, format="%(name)s: %(levelname)s: %(message)s"
         )
         exit_code = 0
         try:

I'll add a fix for that here.

@garlick

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

This looks nice!

@grondo grondo force-pushed the grondo:jobspec-environ branch from 0f3b168 to ef1b17a Jul 12, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Ah, one problem is likely this:

Nevermind. I backed out that change. I clearly don't understand how the python logger works. The patch above fixed a contrived case I hit that issued a log message prefixed with util:, but broke other error messages like this:

$ ../src/cmd/flux jobspec srun -t foo myApp
__main__: ERROR: invalid time limit string format. Acceptable formats include minutes[:seconds], [days-]hours:minutes:seconds

This is fixed by the following obvious change, but I'm not sure if its the python way of doing things:

diff --git a/src/cmd/flux-jobspec.py b/src/cmd/flux-jobspec.py
index cfb642e61..979318250 100755
--- a/src/cmd/flux-jobspec.py
+++ b/src/cmd/flux-jobspec.py
@@ -222,7 +222,7 @@ def get_slurm_common_parser():
     return slurm_parser
 
 
-logger = logging.getLogger(__name__)
+logger = logging.getLogger("flux-jobspec")
 
 
 @util.CLIMain(logger)

(Also not sure if I'm off on the wrong track. Since the problem is only contrived for now, I'll just leave it at that)

@SteVwonder

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

LGTM! Thanks @grondo.

Maybe something to look into there regarding generic exception handling in the util package.

Let me poke at that a bit.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Let me poke at that a bit.

Ok, thanks!

Then I've got nothing more for this PR.

grondo added 3 commits Jul 11, 2019
Add current environment to attributes.system.environment per RFC14 and 24.

Support the srun(1) --export option to control which environment variables
are exported. Default is all.
Problem: a typo in the jq expression was causing jq to assign to
the key instead of testing its value.

Fix the test to use `==` instead of `=`.
Add tests for flux-jobspec handling of attributes.system.environment.
@grondo grondo force-pushed the grondo:jobspec-environ branch from ef1b17a to af6ddbb Jul 12, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Rebased. (Though at this point we could start trying mergify again to avoid manual rebasing)

@codecov-io

This comment has been minimized.

Copy link

commented Jul 12, 2019

Codecov Report

Merging #2227 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2227      +/-   ##
==========================================
+ Coverage    80.7%   80.72%   +0.02%     
==========================================
  Files         202      202              
  Lines       32295    32295              
==========================================
+ Hits        26064    26071       +7     
+ Misses       6231     6224       -7
Impacted Files Coverage Δ
src/common/libflux/message.c 80.75% <0%> (-0.13%) ⬇️
src/broker/module.c 75.17% <0%> (+0.23%) ⬆️
src/common/libflux/flog.c 91.72% <0%> (+0.75%) ⬆️
src/common/libflux/mrpc.c 88.97% <0%> (+1.18%) ⬆️
src/common/libutil/stdlog.c 95.32% <0%> (+2.8%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Is this ready to go in?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Yes, I think this one is ready (got the +1 from @SteVwonder)

@garlick garlick merged commit 313820b into flux-framework:master Jul 12, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch Coverage not affected when comparing cba3138...af6ddbb
Details
codecov/project 80.72% (+0.02%) compared to cba3138
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Thanks!

@grondo grondo deleted the grondo:jobspec-environ branch Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.