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

Environment variable handling #174

Merged
merged 26 commits into from
Jul 5, 2022
Merged

Conversation

OctaveLarose
Copy link
Contributor

Allows specifying environment variables. Related to #42.

The test module is rebench.tests.features.issue_42_env, it's not exactly very elaborate or elegant though.

The syntax looks like this:

Executor1:
  path: .
  executable: foobar
  env:
    - MY_VAR: "VALUE1"
    - BAZ: "FED_AS_AN_STR"

A few notes:

  • only for executors right now, despite the syntax specified in rebench_schema.yml (i.e it being specified in EXP_RUN_DETAILS). Considering adding it to benchmark suites as well, not sure where they'd be most relevant ; as far as I can tell, it should be an easy implementation, though. Let me know where they're needed.
  • the original issue mentioned being able to embed %(input) in env variable values, and that's not implemented there.
  • can specify invalid env variable names since there's no restrictive regex checking their syntax.
  • could possibly add a no_env flag as well to run subprocesses with the bare minimum env instead of passing on the parent process' full environment variables, which it currently does and did even before I added those changes. Doesn't sound useful to me personally, though.

@o-
Copy link

o- commented Mar 29, 2022

Thanks for the contribution, I would also be interested in having this merged.

I tried this PR and unfortunately it fails for executors which have no env key defined:

  File "/usr/local/lib/python3.8/dist-packages/rebench/model/executor.py", line 44, in compile
    env = {key: name for k in env for (key, name) in k.items()}                            
TypeError: 'NoneType' object is not iterable    

Then after being bitten by this I would make:

could possibly add a no_env flag as well to run subprocesses with the bare minimum env instead of passing on the parent process' ...

the default, possibly even the only option!

And a detail: I would allow for keys to be numbers so one does not have to quote them.

@OctaveLarose
Copy link
Contributor Author

Hi, I saw your other issue indeed. Thanks for catching that bug, that's an easy fix - I'll need to add that check to the tests as well.

the default, possibly even the only option!

Yeah, I did run into issues regarding environment passing when I wrote my PR, which is where that suggestion spurred from. That's something I need to discuss with @smarr in the future, but judging by your recent issue, it should at the very least be an option.

And a detail: I would allow for keys to be numbers so one does not have to quote them.

Noted. I remember struggling to make pykwalify work like I wanted it to, so I'm not sure I can implement this easily. I can certainly try, though, although that probably won't be anytime soon FYI.

@o-
Copy link

o- commented Mar 30, 2022

ok, after some experimentation I found out that this PR actually suffers from #189 too. if rebench-denoise is used the specified environment variables are not passed to the benchmarkee.

@smarr
Copy link
Owner

smarr commented Mar 30, 2022

@o- yes, the sudo flag will be needed to pass things on, otherwise it prevents env vars to be visible.

Sorry, a little overloaded at the moment.

smarr added 12 commits July 4, 2022 18:59
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
… library

Signed-off-by: Stefan Marr <git@stefan-marr.de>
…ailing in forks

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
- adapt documentation to reflect that

Signed-off-by: Stefan Marr <git@stefan-marr.de>
…ment

Signed-off-by: Stefan Marr <git@stefan-marr.de>
smarr added 2 commits July 5, 2022 10:47
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
… yet

Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr
Copy link
Owner

smarr commented Jul 5, 2022

I did some more work here, mostly to complete testing but also to always pass an empty environment.

I won't add support for expanding variables yet, since this requires careful work around RunId identity, which is currently only considers the command line.

This means, at the moment, env var support is limited in usefulness since different env vars do not mean different run ids. Thus, if runs only differ by the env var, we won't be able to distinguish them.

But that's for later.

Before this can be merged though, it still needs a test that the env vars are actually passed on across denoise.

  • add test that env vars are passed correctly also when using denoise

Not sure how to test this with a unit test.
It requires sudo rights, which we don’t usually have in CI.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
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.

3 participants