-
Notifications
You must be signed in to change notification settings - Fork 50
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-mini: controlling environment variables included #3141
Comments
It feels like we could do better. @trws added something like the following to
This seems preferable to Slurm's approach. For sophisticated users, perhaps an option could be used to supply a Python function that could act as a filter or would be called in place of the default method to fetch the environment? One question though: what is the use case for no environment (i.e. |
Yeah. I like that suggestion!
My personal use case was to use it in combination with
That's a great question. I don't know. What is the minimal set of env vars that you need? Would anyone ever try According to the srun docs, it looks like
Maybe a good starting point is supporting regex? Strawman: |
I actually do this, or really the moral equivalent |
Thanks @SteVwonder and @trws - that makes total sense. I like the idea of
Great point! In Slurm we added a use-env plugin which allowed flexible control over environment via "named" configurations. (e.g. you could run a job with It occurs to me that we could do something even more powerful by optionally passing the formed jobspec object to a plugin or set of plugins after it is created in |
Oh duh, @SteVwonder already brought this one up in #2875 |
How about this?
Where
As a convenience, the e.g. to clear environment $ flux mini run --env="-*" --dry-run hostname | jq .attributes.system.environment
{} To only propagate the current $ flux mini run --env="-*" --env=PATH --dry-run hostname | jq .attributes.system.environment
{
"PATH": "/home/grondo/git/flux-core.git/src/cmd:/usr/bin:/bin"
} To propagate $ flux mini run --env="-*" --env="PATH=+/foo" --dry-run hostname | jq .attributes.system.environment
{
"PATH": "/home/grondo/git/flux-core.git/src/cmd:/usr/bin:/bin:/foo"
}
$ flux mini run --env="-*" --env="PATH+=/foo" --dry-run hostname | jq .attributes.system.environment
{
"PATH": "/foo:/home/grondo/git/flux-core.git/src/cmd:/usr/bin:/bin"
} The env Just throwing this approach out there. I have a working prototype. |
I like that a lot! The one question I have is w.r.t. ordering of the filters. Do the filters get applied in the order they are provided on the command line? If so, presumably this would produce an empty environment: One alternate semnatic would be to sort the modifications and then apply them in the order you have above:
|
Yeah, the filters would be applied in the order they are specified on the command line. This not only might be slightly more predictable, but would be much easier to implement. However, as you note above this does allow you to undo something you've just done. The alternate semantics you proposed above seem like they would work as well. I can't think of a use case they would not handle. Obviously if you added features to get environment from files and or filter or fetch environment with a program you'd have to be explicit about the order those features are processed in and keep that documented. It might be easier in the long run to just state once that "--env and --env-filter options are processed in the order they are given on the command line". |
Sounds good to me! |
I'd probably go with in order FWIW. If there needs to be some further ordering applied, perhaps the way gcc handles order of arguments would help, where it takes each type of argument in order but does not interleave them. All -I arguments are processed in order, but apply to all files specified for example. All -L are applied before -l. So we could say "all filters, then all env" or something if we had to but just having it be in order seems simplest and probably least surprise for now. As to the rules, I like the concept of each of those, I'm not sure I'm onboard with making it implicit based on a prefix, or perhaps especially the "These strings have the form name=value; names shall not contain the character '='. For values to be portable across systems conforming to IEEE Std 1003.1-2001, the value shall be composed of characters from the portable character set (except NUL and as indicated below)." So... yeah, this is valid:
As is this:
And Most shells don't let you use environment variables like this, so we might get away with just saying "you shall not pass!" these environment variables through this interfaces, but at least the chance we'll get someone who wants to set an environment variable to something starting with a |
Good points. I will back off the append/prepend variants.
Sorry got carried away there. |
FWIW I really like the idea of having easy access to that functionality, especially since it makes it composable multiple times on the same command which you can't really do otherwise, just perhaps expressed with separate flags? |
Yeah, the only reason I was using the admittedly naive syntax was to be able to append all rules to the same internal list, then allow a special "read from file" syntax which allowed the same "rules" to be listed in a file, e.g.:
This can't be accomplished if you require a separate option for each environment manipulation rule. However, there's probably a different, but more sophisticated way to handle env-files. |
Hm, I just discovered
🤷 For now we can leave the append/prepend off and a specific option can be added later as @trws suggests. |
Ok, here's where I ended up with prototype for now:
Allowing "rules" like
Then
Prepend/append on the command line would have to be handled separately since they don't have a "rule" syntax in this scheme. This was just meant as an experiment. I'm willing to go a different direction if there is a perceived need for users to set environment variables with leading |
This should have been closed by #3150 |
AFAICT, we don't have a way to control which environment variables get captured by
flux mini
into the jobspec and passed onto the job.Slurm uses the
--export=<[ALL,]environment variables|ALL|NONE>
argument. Do we want to adopt something similar? Any ideas for a better interface?The text was updated successfully, but these errors were encountered: