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

Virtualenv on macOS not working (fish) #1307

Closed
k-lyda opened this issue Nov 8, 2018 · 30 comments · Fixed by #1313
Closed

Virtualenv on macOS not working (fish) #1307

k-lyda opened this issue Nov 8, 2018 · 30 comments · Fixed by #1313

Comments

@k-lyda
Copy link

k-lyda commented Nov 8, 2018

Please provide information about your setup

  • DVC version: 0.20.1
  • Platform: macOS
  • method of installation: pip

It looks like the commands run by dvc are ignoring currently used virtualenv
screenshot 2018-11-08 at 13 48 32

Steps to reproduce the issue:

$ mkdir dvc_test && cd dvc_test
$ git init
$ virtualenv -p python3.6 .env
$ source .env/bin/activate
$ pip install dvc
$ dvc init
$ which python
$ dvc run which python

The output of two last commands is different, which means, that python... comannd run by DVC is not taking into acount activated virtualenv. I've tested with virtualenv as well as with python venv.

I've done the same steps on linux - it works fine, so it's seems that the issue apears only on macOS

@efiop efiop self-assigned this Nov 8, 2018
@efiop efiop added the bug Did we break something? label Nov 8, 2018
@efiop efiop added this to the Queue milestone Nov 8, 2018
@efiop
Copy link
Contributor

efiop commented Nov 8, 2018

Hi @k-lyda !

I will try to reproduce this later today. Thank you for the feedback!

@shcheklein
Copy link
Member

Hi @k-lyda , was not able to reproduce it on my MacOS machine. Would you mind to share echo $SHELL, is it the same shell that you are using? And echo $PATH as well. Can you also check your shell config (cat ~/.bash_profile in my case) - does it override PATH in some way that is related to anaconda?

We'll try anaconda a little bit later today.

@k-lyda
Copy link
Author

k-lyda commented Nov 8, 2018

Hi @shcheklein, indeed it looks like an issue is with adding anaconda inside shell profile script. I will do some more testing to check exactly what is causing the issue

@k-lyda k-lyda closed this as completed Nov 8, 2018
@shcheklein
Copy link
Member

Thanks, @k-lyda. I'll reopen it because I think it's important design question - the way we execute commands with DVC. It seems that ideally behavior should be the same in both cases, does not matter if you run it via DVC or directly from the command line. May be we should use some shell options to prevent it from re-reading profiles/config. What do you think, @efiop ?

@shcheklein shcheklein reopened this Nov 8, 2018
@efiop
Copy link
Contributor

efiop commented Nov 9, 2018

@shcheklein You are right about it being an env vars overwriting issue by bashrc/zshrc/etc when launching a new instance for command execution inside dvc. I will look into it shortly.

@k-lyda
Copy link
Author

k-lyda commented Nov 9, 2018

@shcheklein, @efiop, as you mentioned, in my case the issue occurs only when I have in my fish config (I use fish shell), line export PATH=<anaconda path>:$PATH. When I comment out this line, DVC works properly.

Best Regards,
Konrad

@efiop
Copy link
Contributor

efiop commented Nov 9, 2018

@k-lyda Oh, no wonder I can't reproduce, I should've asked right away about the shell you were using, but I didn't think it could be anything special 🙂 . Unlike other shells like bash/zsh, which don't parse configs when you invoke them like bash script.sh or bash -c 'mycommand',
fish actually is a special one and reads your configs every time, which is where your issue comes from. This happens because when you run dvc run mycommand, dvc looks at your SHELL env variable and uses it to run your command with your current env, which gets overwritten by fish. And there is currently no option to prevent it from reading those configs 🙁 . We could introduce a special hack for it, that would save environment to some temporary file and then make fish source it before running your command. Effectively something like:

$ printenv &> /tmp/tmpenv.sh
$ fish -c 'source /tmp/tmpenv.sh && yourcommand'
$ rm /tmp/tmpenv.sh

To be more exact, we would need to modify run() method https://github.com/iterative/dvc/blob/master/dvc/stage.py#L419. Would you like to contribute a patch? 🙂

Thanks,
Ruslan

@efiop
Copy link
Contributor

efiop commented Nov 10, 2018

Ok, tried to create a patch for this, but ran into problems when sourcing saved env in fish, since it is really angry about overwriting some read-only vars and stuff like that, overcoming which makes the solution even more hackish, at which point I think it is no longer worth it to mess with, since more elegant workarounds are available(such as removing lines from fishrc that overwrite critical env vars; defining SHELL to something like /bin/bash;). Closing this for now. Please feel free to reopen.

@efiop efiop closed this as completed Nov 10, 2018
@efiop efiop removed the bug Did we break something? label Nov 10, 2018
@efiop efiop removed their assignment Nov 10, 2018
@efiop efiop removed this from the Queue milestone Nov 10, 2018
efiop added a commit to efiop/dvc that referenced this issue Nov 10, 2018
Fixes iterative#1307

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@efiop
Copy link
Contributor

efiop commented Nov 10, 2018

Added a warning just in case: #1313 .

@svenski
Copy link

svenski commented Mar 21, 2019

This might help others that use fish and are confused by the warning message.

The recommended way of setting up conda is not to change the PATH env variable but to source a conda.sh (or conda.fish in this case). Here is a link to conda release notes from 2017-12 about this: https://github.com/conda/conda/blob/master/CHANGELOG.md#recommended-change-to-enable-conda-in-your-shell .

My fish.config has a line: source /opt/miniconda3/etc/fish/conf.d/conda.fish and I don't see the issues mentioned in the original post. I think it is only an issue if you change the PATH.

Ideally, based on this, I'd prefer to remove the warning message since it not needed if you set up conda as recommended.

@efiop
Copy link
Contributor

efiop commented Mar 21, 2019

@svenski the way fish handles env vars is still quite error-prone, even without using conda. I'm afraid that if we remove that warning, users will not know where to start looking for a solution and we'll simply get a lot of bug reports about this same thing. If that warning message is not too annoying for you, I would prefer to leave it as is for now.

@shcheklein
Copy link
Member

@efiop @svenski should we just add a config option to opt out of this warning? And change the message a little bit - point it to a documentation (I can help creating it) that describes this issue, how to workaround, how to opt out.

@svenski
Copy link

svenski commented Mar 21, 2019

@shcheklein - I don't have enough understanding or context of dvc to meaningfully chip in on that decision.

However, from a (fish-shell) user point of view it would be nice, as I get a warning for each stage that I run now. But I also understand if you don't want to complicate things too much. Ideally, it would be fixed for fish, even when changing the PATH, but I see that was attempted and abandoned.

I'm using dvc now for a project so I do plan to learn more about the internal in time, I'll keep it in mind to try to help with fixing this issue if I have the chance.

@efiop
Copy link
Contributor

efiop commented Mar 21, 2019

@svenski Created #1765 for it. Thanks for the feedback! :)

@efiop
Copy link
Contributor

efiop commented Sep 30, 2019

@svenski @k-lyda Guys, we were thinking about maybe making dvc fallback to another shell like sh(usually linked to something simple as dash) or bash when we detect fish, so that we can at least run your commands in the same environment. Maybe even make that configurable. What are your thoughts on that? I'm not that familiar with fish, so it is hard for me to tell how essential fish-specific syntax is for you in your pipelines.

@svenski
Copy link

svenski commented Sep 30, 2019

@efiop , I don't think it would impact my existing pipelines. I don't use fish specific scripts, and if I did, I'd make sure there was a #! path/to/fish in them, so I think this is ok from impact perspective.

However, out of curiosity, can I ask why you want to do this? Are there any other issues with fish? The original issue in raised in this ticket can be avoided by setting up conda correctly (recommended) in fish (https://github.com/conda/conda/blob/master/CHANGELOG.md#recommended-change-to-enable-conda-in-your-shell).

So IMHO I don't think this is issue with dvc, it's with setting up conda in fish. TBH, I would opt for removing the warning unless of course there are other issues with using fish shell that are not mentioned in here.

@efiop
Copy link
Contributor

efiop commented Sep 30, 2019

@efiop , I don't think it would impact my existing pipelines. I don't use fish specific scripts, and if I did, I'd make sure there was a #! path/to/fish in them, so I think this is ok from impact perspective.

Nice to know that. I guess that is that way for most of fish users.

However, out of curiosity, can I ask why you want to do this? Are there any other issues with fish? The original issue in raised in this ticket can be avoided by setting up conda correctly (recommended) in fish (https://github.com/conda/conda/blob/master/CHANGELOG.md#recommended-change-to-enable-conda-in-your-shell).

Just something that came to my mind, so decided to ask for an opinion from fish users :) Glad to hear that properly setting up conda works for you!

So IMHO I don't think this is issue with dvc, it's with setting up conda in fish. TBH, I would opt for removing the warning unless of course there are other issues with using fish shell that are not mentioned in here.

It is not only about conda though, in the linked PR we've been running into other env-related issues and fish is especially prone to those because of the way it handles .fishrc. But it looks like except for conda, fish users are not doing anything crazy with .fishrc. So if it is working out for you right now as is and no one will complain about it, I agree that we should leave it as is for now. About the config option for disabling the warning, it is on our todo list, but I honestly can't tell the ETA for it right now 🙁 That being said, if you would be interested in giving a shot implementing it, we will be extremely happy to help. 🙂

Thank you for the feedback!

@kaiogu
Copy link
Contributor

kaiogu commented Jan 24, 2020

Hi, I have a similar Issue.

The problem was that dvc run was restarting fish shell which activates condas base environment by default. This seems to be a fish shell limitation.
My workaround for conda version 4.7 was:

  • conda init fish on the cli
  • add conda activate my_project_env to ~/.config/fish/config.fishafter the part added byconda init`

Now fish alway starts by activating this specific project environment, but i get to keep everything else.

@efiop
Copy link
Contributor

efiop commented Jan 24, 2020

Hi @kaiogu !

Indeed, that is a fish limitation. We've even put a special warning for dvc run when it detects fish. Unfortunately due to such uncommon behavior of fish where it reloads config files when ran in non-interactive mode, we are not able to mitigate the issue on our side 🙁 So the only workaround is to adjust your fish config to not mess up your current env when it gets reloaded.

@ghost
Copy link

ghost commented May 31, 2020

I don't think we should close this issue, as a warning message is not a solution.
Should I give up fish just so I can use dvc?
By the way, if I run dvc run in a bash it will not help as my default is still fish.
I guess, dvc config should include a shell settings or a conda env setting so that one can run in fish

@ghost
Copy link

ghost commented May 31, 2020

I found the following workaround:
dvc run conda run -n env_name python script.py
this will use conda built in "run a script within an environment context"

@efiop
Copy link
Contributor

efiop commented May 31, 2020

@hanan-vian Do any of the workarounds described above work for you? 🙂

Should I give up fish just so I can use dvc?

Not at all, you just need to configure your setup to make it work.

By the way, if I run dvc run in a bash it will not help as my default is still fish.

That's because your SHELL env var keeps pointing to fish. You could set it to bash or something else and it should work fine.

I guess, dvc config should include a shell settings or a conda env setting so that one can run in fish

Sounds too hackish. The proper solution is probably to make fish support an option (e.g. --norc) that will make it usable, but fish folks seem to be against it fish-shell/fish-shell#5394 . Please feel free to comment on that issue, maybe the pressure from actual users will make them reconsider it.

As I've mentioned previously, we could consider launching bash (or something else if available) if we detect fish, but again that is pretty hacky and might result in some unexpected behavior. It would be great if someone who uses fish could give that approach a try and see if that works for him or not. We would be glad to accept such a patch.

@ghost
Copy link

ghost commented May 31, 2020

@hanan-vian Do any of the workarounds described above work for you?

Should I give up fish just so I can use dvc?

Not at all, you just need to configure your setup to make it work.

By the way, if I run dvc run in a bash it will not help as my default is still fish.

That's because your SHELL env var keeps pointing to fish. You could set it to bash or something else and it should work fine.

I guess, dvc config should include a shell settings or a conda env setting so that one can run in fish

Sounds too hackish. The proper solution is probably to make fish support an option (e.g. --norc) that will make it usable, but fish folks seem to be against it fish-shell/fish-shell#5394 . Please feel free to comment on that issue, maybe the pressure from actual users will make them reconsider it.

As I've mentioned previously, we could consider launching bash (or something else if available) if we detect fish, but again that is pretty hacky and might result in some unexpected behavior. It would be great if someone who uses fish could give that approach a try and see if that works for him or not. We would be glad to accept such a patch.

See my conda run hack.

@horaceg
Copy link

horaceg commented Nov 18, 2021

I use fish + conda (miniforge).

I tried various approaches listed here, and I found only one approach to solve it :
SHELL=/bin/bash dvc ...

To have a smoother process I leverage fish abbrs:

abbr bs SHELL=/bin/bash and then bs dvc ...

@clamydo
Copy link

clamydo commented Apr 21, 2022

Fish gained a --no-config option that could be used, when detecting fish (see fish-shell/fish-shell#7921)

@efiop
Copy link
Contributor

efiop commented Apr 21, 2022

@clamydo Thanks for the heads up! 🙏 We would appreciate a PR if you or anyone else is interested in contributing.

@clamydo
Copy link

clamydo commented Apr 21, 2022

@efiop Today, I started to hack something together. Just adding the option seems easy, however, checking if a fish version above 3.3 is installed and only then apply the option is a tad more fishy. I currently run fish --version in warn_if_fish and check for the version.

But I haven't had time to figure out, how to cleanly adapt the _make_cmd function, without spawning a subprocess everytime. I hope I'll find some time tomorrow.

I think it is necessary to check for the version to remain backwards-compatible, because fish 3.3.x and above isn't necessarily wide-spread yet.

@gerardsimons
Copy link

gerardsimons commented Jul 22, 2022

@clamydo : Do you have a branch somewhere we could look at? I am still running into this issue

I made a fix similar to @horaceg but with an alias. Put this in your fish config

# Work around for dvc / fish / conda bug
alias bdvc="SHELL=/bin/bash dvc"

And then use bdvc instead of dvc

@clamydo
Copy link

clamydo commented Jul 24, 2022

I haven't pushed anything publicly yet because I ran out of time figuring out how to properly handle all cases of different shells and versions.

It's still on my backlog though.

Note (to myself): Fish maintains a FISH_VERSION env variable, no need to spawn a command.

@clamydo
Copy link

clamydo commented Aug 6, 2022

I have started an early attempt to fix this isse. See #8101
I am not happy about this solution. I haven't found away avoiding spawning a command to find out about the shell version.

Perhaps somebody can chime in?

@shcheklein shcheklein changed the title Virtualenv on macOS not working Virtualenv on macOS not working (fish) Aug 6, 2022
clamydo added a commit to clamydo/dvc that referenced this issue Aug 6, 2022
Prevent fish to reload config files in non-interactive mode. Disable warning when using a recent fish version.

Fixes iterative#1307
@oswdm oswdm mentioned this issue May 30, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants