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

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

Open
tandav opened this issue Mar 18, 2023 · 3 comments
Open

Comments

@tandav
Copy link

tandav commented Mar 18, 2023

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',
        }
   )
@mattrasmus
Copy link
Collaborator

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.

@tandav
Copy link
Author

tandav commented Mar 19, 2023

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.

@spitz-dan-l
Copy link
Contributor

spitz-dan-l commented Mar 20, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants