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
Add Python interpreter to execution PATH for AWS Batch #735
Conversation
d579478
to
f5b3b63
Compare
This is needed to ensure non-pythonic conda dependencies are visible to the user code on AWS Batch
1599e3c
to
f5c090f
Compare
# Add the Python interpreter's parent to the path. This is to | ||
# ensure that any non-pythonic dependencies introduced by the conda | ||
# environment are visible to the user code. | ||
env_path = os.path.dirname(sys.executable) |
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.
Can this have unintended consequences in a non Conda env? I don't think so but checking.
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.
This shouldn't since the effect is isolated to an individual task.
@@ -305,11 +313,7 @@ def runtime_step_cli(self, | |||
if self.addl_paths is not None: | |||
addl_paths = os.pathsep.join(self.addl_paths) | |||
python_path = os.pathsep.join([addl_paths, python_path]) |
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.
So here we are assuming that PATH
will be updated later in task_pre_step
, correct? Why not just remove the restriction on batch
?
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.
Correct. Just removing the restriction on batch
won't work since we need to set the environment during task execution and this hook executes pre-task execution.
@romain-intel Good to merge? |
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.
LGTM (well, tests should pass but is it the R issue again)
This is needed to ensure non-pythonic conda dependencies are visible to the user code on AWS Batch