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

cylc command error when running in Anaconda or virtualenv (or both) #2991

Closed
kinow opened this issue Mar 12, 2019 · 8 comments · Fixed by #3006
Closed

cylc command error when running in Anaconda or virtualenv (or both) #2991

kinow opened this issue Mar 12, 2019 · 8 comments · Fixed by #3006
Assignees
Milestone

Comments

@kinow
Copy link
Member

kinow commented Mar 12, 2019

When Cylc is packaged with setuptools, and we execute the cylc entry command, it may get confused and fail, depending on the values of the $PATH variable.

In my case, I have copied ./usr/bin/cylc to ~/bin/cylc. And if that script is the first match for cylc, then everything works.

Now, after enabling Anaconda Python 3, creating a virtualenv, and installing with the code from #2990 , running cylc version --long fails with:

fatal: not a git repository: '/home/kinow/Development/python/workspace/cylc/venv/.git'
No version is set, cylc must be built first. Aborting...

The issue is that the function to find Cylc's version uses the location of the cylc utility to guess where is Cylc's home. Then tries to open the git folder to confirm it exists, and finally executes the git command to retrieve the version.

But when we create the scripts via setuptools, cylc gets installed in the user PATH automatically. And in the case where the user has Anaconda Python, it gets installed in some directory within Anaconda installation. Or if using virtualenv, it might be within the venv folder, added to the $PATH automagically too.

This is breaking the Travis-CI tests in the setup.py pull request, and caused the unit tests to fail in my environment for a while until I realized what was going on.

@kinow
Copy link
Member Author

kinow commented Mar 12, 2019

If we drop the git suffix, and use the version like 8.0.0a1 instead, I think this issue would simply go away.

@kinow
Copy link
Member Author

kinow commented Mar 12, 2019

And even though dropping the git suffix will solve part of the problem, the test battery also uses the profiling package. In the __init__.py in that package, it calls a similar function to open the .git directory, and check if it is a Git directory.

So that one is also failing and blocking setuptools. Not sure what's the best way to fix that one... I guess it's necessary for profiling perhaps? (I don't know much about that part of Cylc yet)

@hjoliver
Copy link
Member

cylc version checks to see if the cylc it is running is a git repo or a release (.git/ present, or VERSION present) so it knows how to determine its own version. (Not sure if that is obvious or not?)

@kinow
Copy link
Member Author

kinow commented Mar 12, 2019

That part appears to be simpler to fix @hjoliver . What about this one https://github.com/cylc/cylc/blob/f5dbc9653353438129c3769ae1d421fde0e85c0f/lib/cylc/profiling/__init__.py#L26? It is also failing.

It calls cylc version --long and retrieves the directory part. So it means profiling has a dependency on the version response... so fixing the cylc version (i.e. removing the git part) would automatically break the profiling package.

Unless there was another function to try - and fail nicely - to retrieve the .git directory of Cylc (if existent)

@hjoliver
Copy link
Member

Our built-in cylc profile-battery system compares performance for given suites between selected versions. I presume that's the reason for the above.

@kinow
Copy link
Member Author

kinow commented Mar 12, 2019

Our built-in cylc profile-battery system compares performance for given suites between selected versions. I presume that's the reason for the above.

Ahh! That would explain it! Thanks @hjoliver . Will look at this tomorrow with more calm and look around the code to see if there's an easy solution that does not break the profile-battery.

@oliver-sanders
Copy link
Member

Apologies for the profile battery! I'll sort it out one day...

@kinow
Copy link
Member Author

kinow commented Mar 13, 2019

No apologies needed @oliver-sanders , I spent most of my day reading its code today, and the great comments. It is a very interesting piece of software within Cylc.

I think the issue happened more due to the way the version is used in Cylc. I found a way to fix the profile battery, but it requires to change how Cylc version is retrieved/stored. Might need your help reviewing what I've done to see if that makes sense.

kinow pushed a commit to kinow/cylc-flow that referenced this issue Mar 14, 2019
kinow pushed a commit to kinow/cylc-flow that referenced this issue Mar 14, 2019
kinow pushed a commit to kinow/cylc-flow that referenced this issue Mar 14, 2019
@matthewrmshin matthewrmshin added this to the cylc-8.0a1 milestone Mar 14, 2019
kinow pushed a commit to kinow/cylc-flow that referenced this issue Mar 14, 2019
kinow pushed a commit to kinow/cylc-flow that referenced this issue Mar 17, 2019
kinow pushed a commit to kinow/cylc-flow that referenced this issue Mar 18, 2019
kinow added a commit to kinow/cylc-flow that referenced this issue Mar 18, 2019
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.

4 participants