Cylc-flow: cylc command error when running in Anaconda or virtualenv (or both)

Created on 12 Mar 2019  路  8Comments  路  Source: cylc/cylc-flow

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.

Most helpful comment

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.

All 8 comments

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

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)

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?)

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)

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

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.

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

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.

Was this page helpful?
0 / 5 - 0 ratings