Skip to content

Add a __version__ key to vp and n3fit#748

Merged
scarlehoff merged 15 commits into
masterfrom
burn_down_tag
May 8, 2020
Merged

Add a __version__ key to vp and n3fit#748
scarlehoff merged 15 commits into
masterfrom
burn_down_tag

Conversation

@scarlehoff
Copy link
Copy Markdown
Member

@scarlehoff scarlehoff commented Apr 28, 2020

There are so many corner cases... but I guess we can consider burning down a string only upon deploy and call the git code otherwise.

I am missing a very important use case* I don't know how to go around:
I have my repository in one folder and run the code from a different folder. So I cannot just "git it". Rely on installing the files in development mode doesn't work either.
I can burn, from setuptools, the path to git...

Anyway, I'd love any ideas for the previous situation.

For the burning the version on the conda package I think what I did is ok (with a bit more logic to go around cases in which there is something written in the __init__) as well as for when the code is ran inside the repo. Any comments?

*it is very important because it is probably the standard case in clusters

Closes #275

@scarlehoff scarlehoff requested review from Zaharid and scarrazza April 28, 2020 20:23
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 28, 2020

You can set the current working directory in subprocess, from Path(__file__).parent. That works for me at least.

I think this is more or less the right approach. A few minor nitpicks:

  • I'd use a separate file called version.py or whatever with a big comment saying to not add anything else in there.

  • I actually tried and am unsure you can get the same format as conda so easily with one invocation of git describe. There are various corner cases, notably the fact that when you are in a tag commit it returns the tag itself without info on the commit hash or a zero for the revision. We could declare that it doesn't matter or even change the logic in conda. Otherwise you probably need some extra logic.

  • You should use text=True and check=True in subprocess.run.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 28, 2020

Also repurposing double dunder names is frowned upon by the thought police. We should call the thing __version__ instead.

@scarlehoff
Copy link
Copy Markdown
Member Author

You can set the current working directory in subprocess, from Path(__file__).parent. That works for me at least.

Only if you did something like python setup.py develop. You might be developing only vp or only n3fit and install the other, then you lose access to the repo.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Apr 29, 2020

I touched a bit the logic to deal with corner cases I could think of (some of the ones you mentioned).

Still missing a satisfactory way to deal with the problem I mentioned before. One thing can be "if you install in development mode you don't have access to the tag, though luck" but the conda installation could be in a different hard drive from the repository which you don't have access to at run time.

Ideally setuptools would change the version.py file on the fly only on install...

@scarlehoff scarlehoff changed the title Add a __build__ key to vp and n3fit Add a __version__ key to vp and n3fit Apr 29, 2020
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 29, 2020 via email

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Apr 29, 2020

I am in a cluster where the conda installation is in some remote disk which all nodes have access to (an amazon s3 bucket for instance). I develop, however, in the local disk of the login machine because otherwise files take a minute to save.

Since this is my workflow I install the package with python setup.py install and then things run in some cluster which have no access back to the repository

Note: one of the clusters I have access to in Milan work like this. I never use it anyway, but it is a case

Note 2: Saying "ok, in that case you still can use cmake and you should" wouldn't count as "satisfactory" but I am ok with that. I prefer the setuptools on the fly modification better but meh.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 29, 2020

I guess this thing can do that, but the README is way too long for me:

https://github.com/pypa/setuptools_scm

@scarlehoff
Copy link
Copy Markdown
Member Author

The trick is to instead of spend 10 minutes reading the readme to spend 4 hours trying and failing!
I'll try. There's 4 hours from now to the code meeting. I'll get it just in time.

@scarlehoff
Copy link
Copy Markdown
Member Author

Apparently setuptools_scm does not work well with pip because of pip moving everything to some /tmp folder before installing.

The easy solution is using setup.py (if it works on travis). Not sure whether there's any downsides to this in our case.

@scarlehoff
Copy link
Copy Markdown
Member Author

"if you install in development mode you don't have access to the tag, though luck"

This is my answer now to all corner I was thinking about before.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 29, 2020

Honestly I much must prefer a simple minded approach that I understand which we had at the beginning than the overenginerred dependincy that triggers problems now and who knows what in the future

@scarlehoff
Copy link
Copy Markdown
Member Author

Well, the thing is that the problem is not weird, given that pip is moving things to a /tmp folder and then you lose access to git. But there is no "easy solution" I can think of for this.

Anyway, I went back to the simple approach. Didn't want to reset and push in case someone wants to go back to setuptools_scm one day and try it out.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 29, 2020

Ideally we would have a valid identifier as per
https://www.python.org/dev/peps/pep-0440/#local-version-segments
which the conda package implements. That is we would have something like

3.4.1874+gc4c7a1f5-dirty

Instead of

3.4-1874-gc4c7a1f5-dirty

It is a bit more work though.

There is also the corner case of being in a tagged commit, where git describe doesn't print the commit or the revision (but it prints "dirty" if you ask for it). We could probably forget about that, in that the

A bunch of lower level commands include (which may or may not make things easier):

#get tag
git rev-list --tags --max-count=1
# c25e438aeb64643d93dd3c1b3c88db21f9ecbffe
#get tag name
 git describe --tags c25e438aeb64643d93dd3c1b3c88db21f9ecbffe
# Get revision number
 git rev-list c25e438aeb64643d93dd3c1b3c88db21f9ecbffe.. --count

@scarlehoff
Copy link
Copy Markdown
Member Author

In theory --long gives you the hash even in the tag case.

The only different between the good and the bad is the "+"?

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 29, 2020

In theory --long gives you the hash even in the tag case.

Cool! that is much better.

The only different between the good and the bad is the "+"?

And the dot for the revision number :)

@scarlehoff
Copy link
Copy Markdown
Member Author

And the dot for the revision number :)

Don't seem worth it... it's a pity git describe doesn't have a --format option though. Damn Linus.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 29, 2020

I would say it is:Tools like to know the revision number, for example to decide which package to install. For example the conda packages used to be problematic when the revision number was a "build number" rather than part of the version, and I still have nightmares.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Apr 30, 2020

Sigh

version=$(git describe --long)
tag=$(git describe --abbrev=0 --tags)
version=${version/${tag}-/${tag}.}
hash=$(git rev-parse --short HEAD)
version=${version/-g${hash}/+g${hash}}

This seems to work fine...

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 30, 2020

Is there no way of having CMAKE doing this when the VP_DEV option is off?

@scarlehoff
Copy link
Copy Markdown
Member Author

Is there no way of having CMAKE doing this when the VP_DEV option is off?

#748 (comment)

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 30, 2020

Right, I thought that was when you were not using cmake at all, but just pip install from the vp directory. I'd say this is a bit more of an issue, but not sure how easy a solution is.

@scarlehoff
Copy link
Copy Markdown
Member Author

The problem is pip sends you to tmp when you install so you lose access to git. From cmake we can change the file in the installation folder after pip has installed.

Something like

echo $(git describe) > $(python -c 'import n3fit.version ; print(n3fit.version.__file__))

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 30, 2020

I like that. I have been trying to get the installation directory from setuptools and now I want to shout.

@scarlehoff
Copy link
Copy Markdown
Member Author

This last commit does the trick.

I am not sure whether it is necessary to write anything from the build scripts anymore, i.e., I am not sure whether to create the package you need it to have the modification in the source file.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 1, 2020

I think this is failing here

mkdir bldtest

because this is happening outside a git repository. One solution is to have the fallback thing you have in python.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented May 1, 2020

I'm happy with the last one, let's see whether travis is also happy

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 1, 2020

If this is now done in cmake, we don't need it in the package scripts right?

@scarlehoff
Copy link
Copy Markdown
Member Author

we don't need it in the package scripts right?

I thought so but then travis failed to find git.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 1, 2020

Argh. I see. I guess conda is not copying the .git folder. I am not sure how much I like having 4 separate mechanisms (conda package, python dev, cmake, conda build), but I also don't have better ideas or feel particularly motivated to think about them.

@scarlehoff
Copy link
Copy Markdown
Member Author

Mm, the linux travis is understanding the repository as "dirty", not 100% sure why... maybe the cmake and the deployment tag need to be set differently...

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 7, 2020

I wouldn't add git to the dependencies. That can do all sorts of bad things, and wouldn't solve much unless you also copy the .git folder. Now I prefer the previous idea of writing the tag both in cmake if possible and in the conda build script.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented May 7, 2020

I wouldn't add git to the dependencies.

Yeah, and it didn't solve the problem anyway (but only in mac for some reason!). Well it was a wild try. Reverting.

@scarlehoff
Copy link
Copy Markdown
Member Author

I'm going to merge this and cross my fingers. If anything breaks I'll revert. If everything works I'll add next the gitstamp to the n3fit replicas.

@scarlehoff scarlehoff merged commit 04af797 into master May 8, 2020
@scarlehoff scarlehoff deleted the burn_down_tag branch May 8, 2020 08:38
@scarlehoff scarlehoff restored the burn_down_tag branch May 10, 2020 16:25
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 this pull request may close these issues.

Add version information at runtime

2 participants