insitro/redun

[feature request] add option to pass environment variables for script task

Opened this issue · 3 comments

It would be handy to pass a dict with environment variables:

@task()
def align_dna_reads_to_genome():
    return script(f"""
        my_command
        """,
        env={
            'KEY_1': 'VALUE_1',
            'KEY_2': 'VALUE_2',
        }
   )

Thanks @tandav for sharing this suggestion. Right now the equivalent workaround is to generate the export commands yourself:

@task()
def align_dna_reads_to_genome(key1, key2):
    return script(f"""
        export KEY_1={key1}
        export KEY_2={key2}
        my_command
        """
   )

Is your suggestion to have script() help prepend such export lines when env is given? I could see that being helpful since there is some subtlety in properly escaping the env var values.

Thanks for workaround example!
I see that scripting.py calls subprocess.run, which have env argument

redun/redun/scripting.py

Lines 78 to 80 in 8df1415

proc = subprocess.run(
command2, check=False, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE
)

It would be nice to support passing env dictionary from script() to that subrocess call. I agree that solution which requires prepending export lines is not robust.

Our team actually has this implemented along with a couple other changes to redun's scripting:

  • script() has an environ= arg that works as suggested above
  • tempdir creation and cleanup is embedded in the generated bash wrapper, so we eliminate passing the temp_path argument around. This allows the tempdir creation to still work for scripts running in a docker container or remotely. (The downside of this is that if your script errors you can't cd into the temp dir to poke around anymore, it is cleaned up automatically on exit. This behavior could be changed though.)
  • Scripts also support redun's code packaging now.

A limitation to just generating the exports directly in your scripts is that not all scripts use shell. E.g. some of our scripts use R too. So we would need to write this logic differently for each language we want to use. If instead the wrapper bash script does the env var exports itself, it generically works for any executable script that it calls.

If there's interest we can factor just the above changes into a PR here.

Edit: For clarity, I'm part of an external team that uses redun and we maintain a private fork, always happy to contribute stuff back.