-
Notifications
You must be signed in to change notification settings - Fork 1
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
first version #1
Conversation
I still don't get the version string. For example. I made changes to use setuptools_scm, committed it and then use it, and I get this version string: '3.3.dev12+ng1a3cb05.d20191210' It should not be 'dirty', and it should give me a distance of one from the latest tag (3.2). It increases 3.2 as expected 3.2 -> 3.3 because there is no patch version, but I don't understand the distance of 12 and the dirty tag. So I was expecting '3.3.dev1+ngd7cb1bd' If I show everything on gitk, this is what I see: So... the hash 1a3cb05 corresponds to the last "blobs_n_frames" commit, which indeed is 12 commits ahead of 3.2. Somehow this version string reflects how it was when I was testing it (uncommitted changes on top of the branch), but since then checked out master (not yet rebase from remote) and committed the changes. What could I be missing? At the same point, the output of 'git describe' is '3.1-9-gd7cb1bd'. This is nine commits ahead of 3.1, hash gd7cb1bd. This is correct, but here I wonder why it did not take 3.2. I was expecting '3.2-1-gd7cb1bd' |
ok, I figured it out... will fix. |
…does not match the actual loaded module
…t_root argument for flexibility
…if loaded module does not match installed module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start. There package should be named ska_helpers as the scope is beyond ACA. Probably more coming but the plane will take off shortly.
setup.py
Outdated
packages=['aca_helpers'], | ||
tests_require=['pytest'], | ||
use_scm_version=True, | ||
setup_requires=['setuptools_scm'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs also 'setuptool_scm_git_archive'
to allow building from a zip file. See sparkles for the template, including those two other files .gitattributes
and the other .git_<something>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
setup.py
Outdated
@@ -0,0 +1,19 @@ | |||
# Licensed under a 3-clause BSD style license - see LICENSE.rst | |||
from distutils.core import setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from setuptools import setup
I can't tell you exactly why, but I think setuptools
is the way to go. For one thing, my astropy colleagues who obsess about infrastructure have put together this guide: https://oa-packaging-guide-preview.readthedocs.io/en/latest/minimal.html. We don't need to fully buy into this immediately, but they have their pulse on the modern idioms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
from distutils.core import setup | ||
|
||
try: | ||
from testr.setup_helper import cmdclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setup_helper
would be a perfect candidate to migrate into this package. We do not need to remove the old one, but if we make ska_helpers
a standard entry in setup_requires
then one can imagine just
from ska_helpers.setup import cmdclass
The ska-helpers
package can be put on PyPI to always be available for a networked machine.
setup.py
Outdated
cmdclass = {} | ||
|
||
setup(name='aca_helpers', | ||
description='Utilities for SKA packages', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SKA
is the Square Kilometer Array. The ska
runtime environment is not an acronym! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
aca_helpers/version.py
Outdated
# this appears to be cyclic, but it is not. | ||
# When called from within __init__.py, the module is not completely defined yet, | ||
# but that is not an issue here. | ||
module = importlib.import_module(package) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A key goal (requirement?) is to avoid import of the module just to get the version. I think you can look at testr.test() for some hints on how to get the upstream ‘file‘. Hopefully that will be fully sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good points, but this one I'm not sure.
- In general I think explicit is better than implicit,
- accessing the upstream file breaks encapsulation and it is not clear from the interface.
- It does not use anything from the module itself (the module is partially initialized), it only gets the file. Maybe there is another way to get the file of a module without importing it?
On the other hand, what you propose depends on the context of the function call. In other words, if I call
get_version('maude')
from some other place, I will get a wrong answer.
After I pushed this, I was thinking that I do not like the current interface because one needs to give the git root as an argument, which is something client code should not be expected to know. Only the maude module knows its relation to the git root. It is again an issue of context.
One way to make the context dependence explicit (although not to remove it) is to pass the file as argument, basically having the 'root' and 'relative_to' arguments from setuptools_scm.get_version.
I would be happy with this partial loading of the module, if I could also then determine whether the file is in a git repository and where the git root is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically I'm OK with context dependence and documenting clearly that get_version
is only intended for use in a very specific context, namely being called from a package top-level __init__.py
, which might be installed or it might be in a git repo. I'd like this particular get_version
to be simple to use in that context, so basically just __version__ = get_version()
in every package except Ska.engarchive get_version('Ska.engarchive')
.
It could be called get_version_in_init
if that helps with being explicit. Sorry that my original description was too vague and you have implemented a more generalized function that can work in any context.
aca_helpers/version.py
Outdated
try: | ||
dist_info = get_distribution(package) | ||
version = dist_info.version | ||
except DistributionNotFound: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be broad Exception. This code should never raise an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I'm also now thinking about your 'import' solution.. apart from not understanding why it is not circular, it might be OK since it is only occurring within an import. |
These are all the package versions (except cheta) with the version changed in my local copy (before git commit). Ska.quatutil, agasc, and kadi are unexpected, and the reason is that the version was changed in the code, but there is no tag yet.
I will push the kadi changes into a branch for you to review, because I do not know how these changes matter to client code. The rest are straight forward. |
I think the Ska.* issues arise from the Ska namespace package and things not being properly installed. What I did was to change PYTHONPATH and import all modules as they were, but this might not be right for Ska.* It's till funny that it does "something". I think it gives the version of the first "Ska" that gets imported. |
|
Looking forward to not having this ambiguity about how and when to change the version number! |
I gave this pounding with all cases I can think of and all looks well. See the top description. The case where there is no git directory but there is a left-over |
But also needs install testing, especially on Windows from a github zip archive. I will do that. |
For this push to improved versioning, do we want this to work in the current flight (the CentOS5 compatible one) or should we just move ahead to shiny? I'm asking because I think that @javierggt has just hit the issue that setuptools_scm_git_archive is not available in our old channels. Might be easiest to just build that package ourselves if we want to make this setuptools_scm transition now. |
For building a distribution (as a precursor to installing) that package (and setuptools_scm) is not required to be conda-installed. Setuptools just pip-installs it in a local directory for the build. You'll find those packages in your .eggs/ cache dir if not already available. So, maybe I'm confused or misinterpreted? I never ran into any problem like that. |
But to be clear we do need |
We already have |
So @javierggt , this might be a case where you can just remove a setuptools_scm_git_archive from the recipe and see how we do. |
Yes, the "pip install" of setuptools_scm_git_archive is into a local area that is in the system path during the package build process. It doesn't affect (or leak into) the final installation of the package. At least that is my understanding. |
But I see the issue now that there are two places to specify build requirements. |
I'm not sure I am understanding. What happened just now was that, while building the conda package, setup.py tried to import setuptools_scm_git_archive because it is one of the requirements. Conda therefore asked me to include this in the conda requirements, and then it is not found. So, this is a direct consequence of it being in the setup requirements at all. I don't know how the pip install in a local area comes about here. |
I suppose some kind of work around would be something that removes setuptools_scm_git_archive from setup requirements only if the package being built is a conda package. |
Here is what happens in a clean repo ( So you are saying that BTW, the example below is
|
OK. We're in the middle of a couple of different updates here. So, our current standard build.sh recipe includes "pip install --no-deps", which I think we explicitly decided upon because we didn't want any surprise dependencies... but it has been a while, and maybe we don't care. My point about |
I thought that About |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective this is good for a 0.1 release. I have already been installing this in various test environments and it seems to work just swimmingly. @jeanconn or @javierggt are there any outstanding issue that require work before a first release?
(And I just rebased my kadi py3-server branch on scm_version because it is so fantastically useful to know the actual version of dev code I am playing with, not just 4.19!) |
Also is there a reason this is private? |
Well, I spoke too soon. For some reason I am not able to install this to a test env on linux (kady). I started with basically a new ska3-flight and got the result below. So I played around with bumping setuptools to the latest, still no joy. The exact same thing seems to be working fine on my Mac, so I'm just confused.
|
this was without installing, right? I will check it. |
I'm pretty sure this is all unrelated to ska_helpers, which is not installed. I found a resolution (but not explanation), which is that upgrading to the latest setuptools_scm version 3.3.3 via pip seems to fix the problem on linux. But note that version 1.15.0 works fine for me on Mac, so the version diff is not a full explanation. And this opened up a different rabbit hole that I really don't get. On both mac and linux I am running conda version 3.4 and have no On mac,
|
I should say that in both cases the channel was listed as Now I just installed conda 4.7 on linux and it found setuptools_scm 3.3.3 in |
I think I might know what's going on... This failure disappears if you remove .git_archival.txt and .gitattributes from the ska_helpers directory. Now... what are those files? you may ask. Those are the required files for setuptools-scm-git-archive. setuptools-scm-git-archive can not be found in the usual repositories. I got it from conda-forge. At that time, we discussed it and figured "maybe some incompatibility arises," but I was hoping that it wouldn't be a problem, because setuptools-scm-git-archive would only be used from github tarballs. In any user installation, there would be no need for it, so there. It seems that, when doing |
so this is a real issue. All packages are missing these files (that enable setuptools-scm-git-archive) and if I add them, I guess this will break the build. Somehow I missed that. |
I propose that we just baseline setuptools_scm 3.3.3 as what will be in ska3-core. We can do that any time since that package is literally not used by any single Ska package at this moment. But we do need to ensure that the conda package is in our icxc ska3conda distribution so conda 4.3 will find setuptools_scs 3.3.3 instead of 1.15 on |
I can confirm that adding setuptools_scm 3.3.3 removes the error. I copied the file from https://anaconda.org/conda-forge/setuptools_scm/files into a directory and added that single directory to my channels for testing. |
Also, probably a silly question, but is " python setup.py sdist" something that needs to work (I don't make packages anymore in our current workflow with this), or was this just a test to see if "everything" sort of worked? Trying to figure out what we explicitly need to test here. |
I don't think we would use it directly, but python setup.py build/install were also failing |
Right, That all being said, if |
Something like this is what I had in mind.
Testing (TLA)
Install latest ska_helpers to a clean Python 3.6 environment
Change to fresh clone of eng_archive with a branch ska-helpers
The
ska-helpers
branch has code to useska_helpers
.Tag a version locally and install
Get out of repo and check version
Back into repo and onto an untagged commit
python setup.py sdist for version 4.48
Downgrade setuptools from version 42 to 36.4
Check again
Test kadi within source repo
Playing with directories