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

Add a Generic "Activation Script" Option #220

Closed
rogermparent opened this issue Mar 31, 2021 · 7 comments
Closed

Add a Generic "Activation Script" Option #220

rogermparent opened this issue Mar 31, 2021 · 7 comments
Labels

Comments

@rogermparent
Copy link
Contributor

rogermparent commented Mar 31, 2021

We've been running into a lot of issues with integrating our extension with the Python VSCode extension (#199) - particularly, the commands that the extension uses to activate a Terminal (e.g. source .env/bin/activate) is not available via API. As we understand it currently, the only way to work around this is on our end by leaning on the Python extension's behavior of automatically activating all integrated terminals. This leads to unpleasant hacks like using an explicit sleep after spawning a Terminal to wait for the Python extension to activate it.

Due to this issue and other considerations like non-Python environments, it may be worth considering completely different approaches and their advantages.

One such approach, tracked by this Issue, would be an "activation script" option that is sourced by the terminal which runs DVC commands. This would allow projects to specify arbitrary, reusable activation instructions required for DVC experiments.

Extra considerations:

  • How would this work on non-Linux systems? Particularly Windows, Mac at least uses the same shell.
    Unfortunately, VS Code does not support platform-scoped config. Allow to scope settings by platform microsoft/vscode#5595 tracks this. We may want to allow for some extra configuration to run different scripts for different values from VSCode's env.shell

  • We'll still need to determine which commands we'll run while activated, and which will be run standalone.
    There's very likely at least a small performance increase to this in an implementation like ours that spawns a shell for each command, but maybe something like an implementation that maintains a persisted terminal with an activated environment would sidestep this completely by only needing to run the activation once and then have it for free for all subsequent commands.

@shcheklein
Copy link
Member

Roger thanks!

the interpreter is exposed, but not the corresponding environment commands that are sent to terminals

could you please expand on this a bit?

@rogermparent
Copy link
Contributor Author

could you please expand on this a bit?

Done! I edited the OP

@shcheklein
Copy link
Member

@rogermparent you mentioned something about exporting the APIs - is it related to this discussion. Was it about asking MS to expose API to get path to the environments?

Also, could you please point to the current implementation (put a link in the Description) - curious to take a look how is it implemented at the moment.

There's very likely at least a small performance increase to this in an implementation like ours that spawns a shell for each command, but maybe something like an implementation that maintains a persisted terminal with an activated environment would sidestep this completely by only needing to run the activation once and then have it for free for all subsequent commands.

I wonder if all these scripts just set specific env variables? It means they run very fast. Also means if we can cache the diff in the env vars and set them on our end?

@rogermparent
Copy link
Contributor Author

@rogermparent you mentioned something about exporting the APIs - is it related to this discussion. Was it about asking MS to expose API to get path to the environments?

At least one instance of me mentioning API exports is very close to what you describe in the second sentence. MS' VS Code extension already exports an API, but that API only allows us to see the path to the Python executable the system uses as opposed to the activation command like source .env/bin/activate.

There's a lot of code written for this environment detection that's used internally throughout the Python extension, like in its terminal activation, but none of that part is accessible via the exported API currently so we're unable to use it to make custom terminals with something like the child_process module.

Also, could you please point to the current implementation (put a link in the Description) - curious to take a look how is it implemented at the moment.

Here is the delay call in IntegratedTerminal, which is what I was alluding to earlier. The fact we've actually resorted to this should show how perplexing this issue has been.

I wonder if all these scripts just set specific env variables?

Yeah, mostly PATH

It means they run very fast.

True in most cases, but we want UI updates as snappy as we can get them, and we already make quite a few calls to dvc.

Also means if we can cache the diff in the env vars and set them on our end?

The main issue here is that we don't have access to information like the required environment variables- all we get right from the Python extension right now is either a path to a python interpreter or a conda run python command.

@mattseddon
Copy link
Member

There's a lot of code written for this environment detection that's used internally throughout the Python extension, like in its terminal activation, but none of that part is accessible via the exported API currently so we're unable to use it to make custom terminals with something like the child_process module.

@rogermparent if / when we create a Pseudoterminal object an activation command is automatically sent from the Python extension and written to the new object. It is possible (but messy) to trap the activation command and send it to a child_process along with && dvc <command>. I did start work on this but it would be better to get the information via the Python extension API if possible.

Here is a screen recording of what happens when you open a Pseudoterminal and run a command in the background. Note that a user cannot interact in any way with a Pseudoterminal:

Screen.Recording.2021-04-01.at.10.04.40.am.mov

This may be an easier piece of work now that we have got code to specifically await a change. My first reaction is that I would prefer not to have running experiments hang off this method. Maybe it is no worse than using a normal Terminal.

@mattseddon
Copy link
Member

Relates to #199.

@mattseddon
Copy link
Member

closing in favour of #267

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

No branches or pull requests

3 participants