Repository navigation
Add support for "conda run" in PythonExecutionFactory - #8259
Kim-Adeline Miguel (kimadeline) merged 70 commits into
Conversation
Kim-Adeline Miguel (kimadeline)
changed the title
Codecov Report
@@ Coverage Diff @@
## master #8259 +/- ##
==========================================
+ Coverage 59.18% 59.25% +0.07%
==========================================
Files 521 522 +1
Lines 23977 24030 +53
Branches 3873 3882 +9
==========================================
+ Hits 14190 14239 +49
- Misses 8870 8875 +5
+ Partials 917 916 -1
Continue to review full report at Codecov.
|
…kimadeline/vscode-python into 7696-python-execution-service-conda-run
Eric Snow (ericsnowcurrently)
left a comment
Eric Snow (ericsnowcurrently)
left a comment
There was a problem hiding this comment.
LGTM (aside from 2 little things)
Thanks for making those changes. I'm approving the PR, but would still like you to consider the 2 comments before merging. :)
091957a to
b6b3fd3
Compare
| public execObservable(args: string[], options: SpawnOptions): ObservableExecutionResult<string> { | ||
| const opts: SpawnOptions = { ...options }; | ||
| // Cannot use this.getExecutionInfo() until 'conda run' can be run without buffering output. | ||
| // See https://github.com/microsoft/vscode-python/issues/8473 |
There was a problem hiding this comment.
Please ensure you use createActivatedEnvironment instead of procService.
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.
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.
Where is that old hack? I could only find the following createActivatedEnvironment calls on master:
In this PR execObservable and execModuleObservable use the old code, I've only added a comment.
There was a problem hiding this comment.
Further to this, createActivatedEnvironment was created to get around conda 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 uses condaRun.
We have a couple of options:
- Return conda execution service when calling
createActivatedEnvironment - Drop
createActivatedEnvironemnt(or make it private) - Anything that requires observable output, then the
conda executionservice will fallback to using the service returned bycreateActivatedEnvironment. - Please remember,
conda runisn't the only special case. We could havepyenvand others.pyenvrequires initializing an environment variable to run the correct python interpreter (they use shims). - Hence
conda runis 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)
|
hurray! |
5 participants
First PR for #7696 (terminal execution update will happen separately)
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)