Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 support for "conda run" in PythonExecutionFactory #8259
Add support for "conda run" in PythonExecutionFactory #8259
Changes from 67 commits
66468a0
4fda3b0
73bee3c
d68445e
de9dddb
16e5542
6d91b33
2646ea2
8296bd0
389bb59
a79252a
bdaa375
865a216
e6b37c3
e294b4d
5d8a1d3
82eafec
9b8f948
780538e
0068e0d
637be5f
ad42ee8
5255e45
6a0ab56
2a43052
3b304be
1fea07e
1361103
d47072d
fe5601b
45160bf
7319345
3e972ac
4f7cc14
1719adf
1b3eb47
2ffe187
1df9f8a
faf1f39
6a772bf
938ebd6
689bf1f
42ba9ff
923dea2
365c92a
77fac1f
fd4ddd6
6edb1da
cf3ba57
5fb07a4
61d685a
55f96e9
77d94a1
68216fa
ee42991
3442437
a1ae719
6d176bc
eb7625b
5dbd096
9c5724e
38c4321
75076e4
a4f0da5
b6b3fd3
610fb01
eddcf15
269c54b
394c85f
0f93c43
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please ensure you use
createActivatedEnvironment
instead ofprocService
.Else this is totally useless.
When running conda we must either use
conda run
or at a minimium revert to the old hack, activate the environment then grab the env variables and run in the context of those variables.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.
Because today we use
createActivatedEnvironment
when running tools such as pytest, unittest, etc.If we are going with conda run then
createActivatedEnvironment
becomes obsolete, BUT ONLY if this (above mentioned) change is added.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.
Where is that old hack? I could only find the following
createActivatedEnvironment
calls onmaster
:In this PR
execObservable
andexecModuleObservable
use the old code, I've only added a comment.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.
Further to this,
createActivatedEnvironment
was created to get aroundconda run
. It allows code to execute python code within an activated python environment context (which is what conda run does).So, anywhere
createActivatedEnvironment
is being used, we need to ensure it usescondaRun
.We have a couple of options:
createActivatedEnvironment
createActivatedEnvironemnt
(or make it private)conda execution
service will fallback to using the service returned bycreateActivatedEnvironment
.conda run
isn't the only special case. We could havepyenv
and others.pyenv
requires initializing an environment variable to run the correct python interpreter (they use shims).conda run
is an implementation detail when it comes tocreateActivatedEnvironment
.Finally, i'd prefer if these were done as separate PRs (issues), as opposed to doing them all in one go in a large PR...I.e. first implement conda exec service, then another, then integrate in a few places, etc... Else it just feels too large (to review)
@karthiknadig /cc
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.
Lets talk