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 conda-wrappers recipe #1205

Merged
merged 1 commit into from
Sep 29, 2016
Merged

Conversation

gqmelo
Copy link
Contributor

@gqmelo gqmelo commented Aug 9, 2016

conda-wrappers mainly aims to make it easy to use conda environments with IDE's, so that you don't need to start the IDE on an activated environment

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/conda-wrappers) and found it was in an excellent condition.

@jakirkham
Copy link
Member

Could you please explain a bit about this post-link scripts? For instance, why did you chose post-link scripts as opposed to having something in the build? Also do we need some sort of pre-unlink scripts? Also it would probably be helpful to give issue ( conda-forge/conda-forge.github.io#173 ) a read-through.

- python
run:
- python
- exec-wrappers 1*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be 1.*. Also why this particular pinning?

Copy link
Contributor Author

@gqmelo gqmelo Aug 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be 1.*

Are you sure? On previous conda-build versions the dot had to be omitted, otherwise it never was able to match any version

Also why this particular pinning?

I intend to respect semantic version with exec-wrappers so that new versions will be backward compatible. As this package depends on the first version, it does not really matter which one is exactly. It could be anything between >= 1.0.0 and < 2.0.0. Maybe it is better to use this expression instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be 1.*

Are you sure, on previous conda-build version the dot had to be omitted, otherwise it never was able to match any version

Yep, works great. We do it with lots of things.

Also why this particular pinning?

I intend to respect semantic version with exec-wrappers so that new versions will be backward compatible. As this package depends on the first version, it does not really matter which one is exactly. It could be anything between >= 1.0.0 and < 2.0.0

❤️ That sounds amazing. I wish everyone did that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, works great. We do it with lots of things.

Good to know it is working, I prefer using the dot. But the semantic of 1* and 1.* is the same, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite 1* could mean 10 or similar. This could cause for some strange surprises later. Hence we have preferred the ..

If the . is not always there, we do something like 1|1.*.

@gqmelo
Copy link
Contributor Author

gqmelo commented Aug 9, 2016

Could you please explain a bit about this post-link scripts? For instance, why did you chose post-link scripts as opposed to having something in the build?

Because the actual content depends on what is installed on the current environment. For example, when you do conda install -n foo-env conda-wrappers it will create wrappers for the binaries installed on foo-env, this is not an info that I have during build.

Also it would probably be helpful to give issue ( conda-forge/conda-forge.github.io#173 ) a read-through.

Thanks for the reference. So according to what was discussed there and the new documentation I am misusing the post/pre link/unlink scripts.
In this case I'm OK with changing the way these wrappers are created.

Any suggestion? By now I can only think of having a command that the user need to explicitly call.

@jakirkham
Copy link
Member

Could you please explain a bit about this post-link scripts? For instance, why did you chose post-link scripts as opposed to having something in the build?

Because the actual content depends on what is installed on the current environment. For example, when you do conda install -n foo-env conda-wrappers it will create wrappers for the binaries installed on foo-env, this is not an info that I have during build.

Ok, makes sense. How do you plan to keep track of which ones to remove and ensure they get removed?

Also it would probably be helpful to give issue ( conda-forge/conda-forge.github.io#173 ) a read-through.

Thanks for the reference. So according to what was discussed there and the new documentation I am misusing the post/pre link/unlink scripts.
In this case I'm OK with changing the way these wrappers are created.

Any suggestion? By now I can only think of having a command that the user need to explicitly call.

Yeah, that is a pretty restrictive look at the link/unlink scripts. Don't think that discussion is really over. For instance, look at what is required to get the gcc install to behave correctly.

So I don't have any problems with using the link scripts the way you are. In fact, what you are doing seems pretty cool and very useful. You may be interested in conda-execute which has another take on this sort of problem.

It is true an explicit command(s) may work too (and may have some value for re-runs for example). However, if you still want that command called in the link scripts, that makes sense to me. Many people are hiding the complexity of their linking scripts by doing exactly that. Just would suggest using the .messages.txt for output/error messages. Also proceed carefully as link scripts are a bit finicky from past experience. Still they are a necessary tool for many use cases.

@gqmelo
Copy link
Contributor Author

gqmelo commented Aug 9, 2016

How do you plan to keep track of which ones to remove and ensure they get removed?

Unfortunately I could not figure out a practical way to do that. conda does not provide a way to know when a package is removed. For example, if gcc was present when conda-wrappers was installed and then gcc is removed, the gcc wrapper will keep existing and try to run a command that do not exist any more.

What I could do is to remove all files when conda-wrappers is removed as a post-unlink script

You may be interested in conda-execute which has another take on this sort of problem.

Yeah, I checked conda-execute. It is pretty cool but it does much more than I need. All I need is a way to run an executable without having to activate the conda env. And that should have a low overhead because on an IDE, for example, if you configure the python wrapper as the interpreter it may be called hundreds of time for indexing purposes.

It is true an explicit command(s) may work too (and may have some value for re-runs for example). However, if you still want that command called in the link scripts, that makes sense to me.

Ok then, I will keep using the link scripts, but encapsulate on separate command

@jakirkham
Copy link
Member

@msarahan, could you share some thoughts on this? It seems like @gqmelo has a pretty cool idea he is pursuing, but there are some challenging points w.r.t. uninstall packages. Do you have any advice? Are there some changes we could/should purse in conda to make this easier?

@jankatins
Copy link
Contributor

The https://github.com/Cadair/jupyter_environment_kernels/ has solved a similar problem but from the other side: the kernels are always executed in a activated environment (at least the env vars are there). So it is possible to fix this on the side of IDEs.

Another idea would be to add such capabilities to conda itself: if a executable script/command is installed into an env, move that script to a different place and replace it with a wrapper which activates the env and calls the moved script/command.

@msarahan
Copy link
Member

msarahan commented Aug 9, 2016

Sorry, I don't have any opinion or suggestion here. I recommend raising this as a conda issue on the conda issue tracker, so that it has higher visibility to @kalefranz

@gqmelo
Copy link
Contributor Author

gqmelo commented Aug 9, 2016

So it is possible to fix this on the side of IDEs.

Yes, it is possible, but I don't expect that every IDEs will properly integrate with conda some day. Even the only one I know that supports conda does not do it properly: https://youtrack.jetbrains.com/issue/PY-18865

Also just making IDEs activate the environment is too slow. You can see some numbers in https://github.com/gqmelo/exec-wrappers . So basically every IDE would come up with its own implementation.

Another idea would be to add such capabilities to conda itself

Yes, that is what I really wanted. I created an issue for that, but it is on hold for now. That is why I decided to publish this on conda-forge, to see if more people start using it and then come up with a builtin solution.

@jankatins
Copy link
Contributor

Just had a look at exec-wrapper. IMO the main benefit is that the needed codepath in activatefor interactive usage make the basic usage much slower and therefore using the wrapper (with a subset of the normal activate script) is making it faster than say a command.bat like activate <env> && command $*.

I like that but it's only useable as long as you explicitly use it as a whatever.bat instead of plain whatever + setting the PATH. The problem here is that at least on windows *.bat are not the same as *.exe, e.g. try starting python.exe vs python.bat via subprocess.check_call(["python"], shell=False) (without extension but with setting PATH accordingly beforehand) -> the shell=False results in python using some Windows API to find the command, but that only finds python.exe when searching for the executable for python as the default (I don't remember if it actually can run bat files or you would have to prepend cmd) (See this mpl bugreport about it). This problem meant that packages like miktex use a natively compiled wrapper just to start programms in different places.

So IMO adding wrapper to the envs (which the user has to explicitly call) or using something like run_activate.bat which activates and then calls the argument have the same implementation overhead on the user side. But the latter would be easily added in the env creation (conda already adds specific <env>\Skripts\{de,}activate.bat files) and would not need to be run after all packages have been installed/updated. So IMO the run in functionality is a nice addition to conda (and other env managers?), I would really like to see that in coda itself!

Unfortunately, in most cases (on windows), it wouldn't make a difference because e.g. in the environment kernel manager mentioned above, the jupyter notebook uses Popen with an implicit shell=False and so still can't use it :-(

For that to work (and having it in PATH), the wrapper would have to be a natively compiled wrapper (+ moving the real command to a subdir and then calling from there?) but that is really bad because it would need to pull out the changes to the environment from the activate.d stuff which would then again be slow :-(

@gqmelo
Copy link
Contributor Author

gqmelo commented Aug 10, 2016

I like that but it's only useable as long as you explicitly use it as a whatever.bat instead of plain whatever + setting the PATH. The problem here is that at least on windows *.bat are not the same as *.exe, e.g. try starting python.exe vs python.bat via subprocess.check_call(["python"], shell=False)

Yeah, unfortunately that is true on Windows. bat files are not exactly executables whereas on Unix'es all it matter is the executable permission. I tend to workaround this issue calling subprocess.call([shutil.which("python")]) on Python 3 (or whichcraft.which("python") on Python 2.7) as this works both on Linux, OSX and Windows regardless of the shell parameter. But of course this option is only valid if you can change the code.

I don't remember if it actually can run bat files or you would have to prepend cmd

It can if you pass the .bat extension (or the full path as returned by the which example above). Not sure what happens under the hood though.

This problem meant that packages like miktex use a natively compiled wrapper just to start programms in different places.

I thought about generating compiled wrappers if necessary, but bat files worked just fine for my use case and my coworkers', which is Eclipse+PyDev and PyCharm. But I am definitely willing to review that if this can benefit other people.

So IMO the run in functionality is a nice addition to conda (and other env managers?), I would really like to see that in coda itself!

I agree that at least this should be present on any env manager and apparently more people agree too. Imagine if docker didn't offer an option to run a command and you had to always open an interactive shell inside the container to run anything.

For that to work (and having it in PATH), the wrapper would have to be a natively compiled wrapper (+ moving the real command to a subdir and then calling from there?) but that is really bad because it would need to pull out the changes to the environment from the activate.d stuff which would then again be slow

I don't think it would be too slow. Actually the wrappers created by exec-wrappers call all activate.d scripts and the overhead is slow enough (provided the activate.d do not do anything complex). Just as an example, on the company I work for we have some activate.d scripts that do a lot of fancy stuff and adds about 80ms of overhead. Even in this case, the wrappers are totally usable. But of course that will depend on your use case. So I think that changing to a compiled wrapper on Windows may be a viable option.

@jankatins
Copy link
Contributor

I don't think it would be too slow. Actually the wrappers created by exec-wrappers call all activate.d scripts and the overhead is slow enough (provided the activate.d do not do anything complex).

I should have been more explicit: I imagine it is slow because it would have to generate a batch file which is then executed by a started cmd process (or do the activate.d logic in the wrapper and call a cmd for all activate.d files).

@gqmelo
Copy link
Contributor Author

gqmelo commented Aug 10, 2016

I should have been more explicit: I imagine it is slow because it would have to generate a batch file which is then executed by a started cmd process (or do the activate.d logic in the wrapper and call a cmd for all activate.d files).

Yes, it will be slower because of that, but I don't think it will matter that much. Anyway I will try to benchmark this

@jakirkham
Copy link
Member

So the performance discussion is interesting and am happy that this discussion is going on. However, IMHO this is not a blocker for packaging this. The reason I say this is performance is not a packaging concern, but a code optimization concern that would be best addressed at the repo for the code.

The only thing I'd really like to see is a proper cleanup mechanism and then we can get this out there. Given we are in the business of package management here, this seems in scope IMHO.

Generally, this tool is quite powerful and has a lot of advantages (especially if it is reasonably fast). The sooner we get it out there the better.

To return to the clean up question, I'd like to note this comment.

What I could do is to remove all files when conda-wrappers is removed as a post-unlink script

This sounds totally sensible to me @gqmelo. Just brainstorming a bit here, what if there is a directory like $PREFIX/share/conda-wrappers. In that directory, it could touch a file when a new wrapper is added. From this one should be able to determine what the wrappers are and clean them up in this post-unlink step (along with $PREFIX/share/conda-wrappers at the end). Does this seem doable? Do you have other thoughts on how to do this?

@gqmelo
Copy link
Contributor Author

gqmelo commented Sep 22, 2016

@jakirkham, thanks for reviving this issue. I promised to play with native wrappers on Windows but did not have time yet and thought this was a blocker.

But actually this would be a feature for exec-wrappers and I created an issue to address that: gqmelo/exec-wrappers#12

Generally, this tool is quite powerful and has a lot of advantages (especially if it is reasonably fast).

Yeah, at least for me it is being really useful. It is the less intrusive way I found to work with many environments on IDE's and debuggers.

In that directory, it could touch a file when a new wrapper is added. From this one should be able to determine what the wrappers are and clean them up in this post-unlink step (along with $PREFIX/share/conda-wrappers at the end)

I don't think this would help. Because this will be executed when the conda-wrappers package is removed. In this case, it should just remove all wrappers, which happen to be on the same dir. So I would just remove the whole wrappers dir.
Actually I would like to remove only the respective wrappers when any package is removed, but I don't see a way to do that.

Anyway, I'll will try to properly cleanup the wrappers on pre-unlink

@gqmelo
Copy link
Contributor Author

gqmelo commented Sep 23, 2016

Anyway, I'll will try to properly cleanup the wrappers on pre-unlink

Done

@jakirkham
Copy link
Member

Actually I would like to remove only the respective wrappers when any package is removed, but I don't see a way to do that.

Good point. Had forgotten about this.

At least now one can effect that change by uninstalling and reinstalling conda-wrappers. It is a bit inelegant, but it at least provides the relevant switch.

It likely requires some changes in conda and packages thereof to register for an uninstall event.

Alternatively, instead of [un]installing the wrapper scripts, you could offer a subcommand format. Though I don't how practical/efficient that is.

In any event, I'm hopeful that giving this more exposure by packaging it on conda-forge that the userbase will grow and draw more attention to this issue.

@jakirkham jakirkham merged commit b578844 into conda-forge:master Sep 29, 2016
@jakirkham
Copy link
Member

Thanks again for putting this together, @gqmelo.

@gqmelo
Copy link
Contributor Author

gqmelo commented Sep 29, 2016

In any event, I'm hopeful that giving this more exposure by packaging it on conda-forge that the userbase will grow and draw more attention to this issue.

Yeah, that is the main reason I wanted to package it on conda-forge, to make it easier to use and see if it becomes popular and then come up with a better solution, more integrated with conda itself.

Thank you!

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

Successfully merging this pull request may close these issues.

5 participants