-
Notifications
You must be signed in to change notification settings - Fork 144
Fixes env vars in languages other than Python #108
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
base: main
Are you sure you want to change the base?
Fixes env vars in languages other than Python #108
Conversation
template/server/messaging.py
Outdated
+ f"os.environ.set_envs_for_execution({vars_to_set})\n" | ||
+ code | ||
) | ||
env_vars_snippet = f"os.environ.set_envs_for_execution({vars_to_set})\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.environ.set_envs_for_execution function might no longer be required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it tries to solve a more general issue. If you only set envs then you have a problem with changing a variable during a runtime, which is also a global variable.
Try this code, if it works (it's from test, which was for some reason commented out)
sbx = await AsyncSandbox.create(envs={"FOO": "bar", "SBX": "value"})
await sbx.run_code(
"import os; os.environ['FOO'] = 'bar'; os.environ['RUNTIME_ENV'] = 'async_python_runtime'"
)
result = await sbx.run_code("import os; os.getenv('FOO')", envs={"FOO": "baz"})
assert result.text == "baz"
# This can fail if running in debug mode (there's a race condition with the restart kernel test)
result = await sbx.run_code("import os; os.getenv('RUNTIME_ENV')")
assert result.text == "async_python_runtime"
result = await sbx.run_code("import os; os.getenv('SBX')")
assert result.text == "value"
result = await sbx.run_code("import os; os.getenv('FOO')")
assert result.text == "bar"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current limitation will be that any env variable that you pass through envs will either be removed or reset to global value, this is breaking current implementation, but i think more of an edge-case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, this
await sbx.run_code("import os; os.environ['FOO'] = 'bar'")
# Will be bar
await sbx.run_code("import os; os.getenv('FOO')")
# Will be baz
await sbx.run_code("import os; os.getenv('FOO')", envs={"FOO": "baz"})
# Will be "None" or the value set in Sandbox.create
# Alternatively, I can make it keep the value from previous step
await sbx.run_code("import os; os.getenv('FOO')")
@mishushakov can you add tests for this case? it would be good to catch if setting env var breaks |
yes, these will be in a separate PR as otherwise we would be unable to merge this (the CI is using deployed template version) |
Changelog:
Example:
Before:
After: