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

Releasing v0.3.3 to fix conda-forge build issue #170

Closed
lsetiawan opened this issue Feb 4, 2021 · 40 comments · Fixed by #179
Closed

Releasing v0.3.3 to fix conda-forge build issue #170

lsetiawan opened this issue Feb 4, 2021 · 40 comments · Fixed by #179

Comments

@lsetiawan
Copy link
Contributor

Overview

After merging #168. I think it's good to release a minor version 0.3.3 to resolve conda-forge/staged-recipes#13866. Since requirements.txt and LICENSE files are missing from the package.

Build error for conda package can be found here.

@lsetiawan
Copy link
Contributor Author

Seems like the recent use of setuptools_scm is causing versioning error on the pypi side, maybe the format of dev versions need to change: https://travis-ci.com/github/icesat2py/icepyx/builds/216001973#L761

@weiji14
Copy link
Member

weiji14 commented Feb 4, 2021

Oops. So we need to use the no-local-version scheme when uploading to PyPI. See documentation at https://github.com/pypa/setuptools_scm#version-number-construction and https://github.com/GenericMappingTools/pygmt/pull/695/files#diff-75e8bbdd253f4a71a14114808840e63b6a9c23529499687d148025d860336504R45 for an example on how to fix the CI.

@JessicaS11
Copy link
Member

When you say requirements.txt and LICENSE files are missing, do you mean in the recipe (or PyPI build that the recipe is pulling from), since they exist in the repo itself? For my own future reference, if these files are updated in the icepyx repo, will I need to also make those changes to the conda-forge recipe?

Does setuptools_scm automatically do a new PyPI build every time something is pushed or merged? I was surprised to see that being the cause of the failed Travis build because pushes/PRs to the development branch should only trigger a PyPI_test build, and only if triggered in the commit message.

Thanks for the links for CI fixes. It's getting pretty late here for today but I'll work on this tomorrow.

@lsetiawan
Copy link
Contributor Author

When you say requirements.txt and LICENSE files are missing, do you mean in the recipe (or PyPI build that the recipe is pulling from), since they exist in the repo itself?

It is missing from the source file that gets build from python setup.py sdist. This is causing an error, since in setup.py you read that requirements.txt. The reason that right now pip install is fine is because you provide a wheel, so it's installing it from there. You can see that if you try to do pip install icepyx-0.3.2.tar.gz it will actually fail. Also, it doesn't seem like LICENSE file is in that tar.gz source file. Having it in there will prevent the need to updating the license file in conda-forge recipe, plus it's a better practice 😄

Does setuptools_scm automatically do a new PyPI build every time something is pushed or merged?

I don't think it does automated build, but @weiji14 can chime in on that.

@lsetiawan
Copy link
Contributor Author

Seems like everything is now included in the source distribution file, which makes it so bloated ... After being so confused as to the cause, I finally tracked down that it was because of setuptools_scm.

I didn't know by default this includes all the files being tracked by git. Seems like it's a known issue: pypa/setuptools-scm#516. Also here's a text about its behavior: https://github.com/pypa/setuptools_scm#file-finders-hook-makes-most-of-manifestin-unnecessary.

@weiji14 do you know how to mitigate this. We could list all the files to exclude in MANIFEST.in I suppose. This is something that should be addressed before the next release. Thanks!

@JessicaS11
Copy link
Member

Also, it doesn't seem like LICENSE file is in that tar.gz source file. Having it in there will prevent the need to updating the license file in conda-forge recipe, plus it's a better practice 😄

I'm all about better practice and non-manual updating! Thanks for the explanation. It sounds like a manifest.in file is a step up from what they show in basic "make your code an installable Python package tutorials", so I never set one up.

I don't think it does automated build, but @weiji14 can chime in on that.

If setuptools_scm doesn't automatically do a PyPI build, then why was one triggered in Travis? The relevant portion of the .travis.yml file is:

deploy:
  - provider: pypi
    server: https://test.pypi.org/legacy/ 
    username: "__token__"
    password: $test_PyPI_API_Token
    on:
      branch: development
      if: commit_message =~ test_pypi
    skip_existing: true
    distributions: "sdist bdist_wheel"

It's been challenging setting up these automations and troubleshooting them (I know there are some issues with the automatic PyPI builds when I tag a new release that I haven't yet sorted out), so any corrections/fixes are welcome!

What files (or types of files) that are tracked by git would normally be excluded from the sdist (but that setuptools_scm includes) to reduce the bloating?

@JessicaS11
Copy link
Member

Also, I started a "conda" branch for these edits.

@weiji14
Copy link
Member

weiji14 commented Feb 4, 2021

What files (or types of files) that are tracked by git would normally be excluded from the sdist (but that setuptools_scm includes) to reduce the bloating?

Ok, so in MANIFEST.in, my understanding is that we should just include the icepyx/ folder, the LICENSE and the requirements.txt file? I.e. exclude the big jupyter notebooks in the examples and doc folders?

Also, I started a "conda" branch for these edits.

Cool, I can chime in and have a look. I'm a bit more familiar with using the PyPI Publish Github Action than Travis CI but it should be quite similar.

@weiji14
Copy link
Member

weiji14 commented Feb 16, 2021

Hi @JessicaS11, just checking up on this, how are you doing with the "conda" branch?

@lsetiawan
Copy link
Contributor Author

#172 Should make the package slimmer. I got about 42kB now with the PR.

Screenshot from 2021-02-16 10-49-20

@JessicaS11
Copy link
Member

I'm scratching my head a bit on this not being able to see the chicken versus egg today. The goal is to release a new version (0.3.3) of icepyx, which will (if all goes according to plan) trigger (independently): 1. a new build and release on PyPI; 2. a package build and availability through conda. We've added setuptools_scm to take care of the version numbering automatically, which is triggered to start with by importing the version in the top level __init__.py file (right?). The conda package build will happen whenever the "newest" version changes relative to the previously "current" version (right?), but I'm not sure how conda-forge knows about it (does it check regularly?). We need to make the same thing happen for PyPI via Travis (I think by setting, or making sure it's getting set correctly automatically, the $TRAVIS_TAG environment variable). The part I'm having the most trouble with is:

  1. where do git and git tags come into this (since before I was triggering the releases by manually adding a git tag)?
  2. how does setuptools_scm know when to up the version versus just leave things alone (e.g. a merged PR to master, but not necessarily every PR that's merged into master, and basically never on any other branch)? I don't think we set up any GitHub actions to do this...
  3. how do I know what the version number is by looking at the code (i.e. no git tags), or is the only way to tell by importing the package?
    Thanks for helping me learn!

@weiji14
Copy link
Member

weiji14 commented Feb 18, 2021

I'm scratching my head a bit on this not being able to see the chicken versus egg today.

Yeah there's a lot of pieces going together. The release pipeline should go as follows:

  1. Merge the development branch into the master branch.
  2. Draft and create a new release on https://github.com/icesat2py/icepyx/releases. By clicking on the green 'Publish Release' button, GitHub will automatically create a tag on the current commit (on the master branch in this case).

image

  1. Since the tag is created, the Continuous Integration service (e.g. Travis) will then build and publish a wheel to PyPI.
  2. After a period of time, the conda-forge bot will detect that there is a new icepyx release on PyPI, and generate an automated PR to bump up the icepyx version on conda-forge/icepyx-feedstock. Merging the PR will make a new conda icepyx package.

To answer your other questions:

We've added setuptools_scm to take care of the version numbering automatically, which is triggered to start with by importing the version in the top level __init__.py file (right?).

Yes. The version string (e.g. v0.3.2) should be automatically created on import icepyx, and people can get is using print(icepyx.__version__). This does assume that the icepyx git history is available, OR for a published release from PyPI/conda, it should be apparent in an _icepyx_version.py file.

The conda package build will happen whenever the "newest" version changes relative to the previously "current" version (right?), but I'm not sure how conda-forge knows about it (does it check regularly?).

See point 4 above. The conda-forge bot detects new releases at PyPI and triggers a PR to bump up to a new version automatically. But of course, we need conda-forge/staged-recipes#13866 first for the initial conda-forge v0.3.3 package. Future releases >=v0.3.4 will be automated.

We need to make the same thing happen for PyPI via Travis (I think by setting, or making sure it's getting set correctly automatically, the $TRAVIS_TAG environment variable).

See point 3 above. Actually, were the previous releases uploaded to PyPI by Travis CI or were they done manually or via another way? If there's no strong preference for using Travis CI, I can make a Publish to PyPI Github Actions workflow as I've got more experience on that. It will also publish test releases to TestPyPI so we can test things before doing the real release.

The part I'm having the most trouble with is:
1. where do git and git tags come into this (since before I was triggering the releases by manually adding a git tag)?

See point 2 above. If I understand correctly, before, you made a git tag, pushed it to GitHub, and then published a release? You can also make a GitHub release directly on the GitHub releases page and this does both automatically in one click (tag and release).

2. how does setuptools_scm know when to up the version versus just leave things alone (e.g. a merged PR to master, but not necessarily every PR that's merged into master, and basically never on any other branch)? I don't think we set up any GitHub actions to do this...

Every commit has a version. There are published versions like v0.3.0, and non-published interim versions like v0.3.0.dev1+abcdef (the 1st commit after v0.3.0). The abcdef part is a unique hash that will change depending on what branch you are on, and so there won't be version string clashes across branches.

3. how do I know what the version number is by looking at the code (i.e. no git tags), or is the only way to tell by importing the package?

If you have the full icepyx git history, you can do git describe --tags which will print out something like v0.3.1-7-gb1db8cb. Or as you said, just import the icepyx library and do print(icepyx.__version__).

   Thanks for helping me learn!

Feel free to ask more questions!

TLDR: Merge development into master -> Tag Release (on GitHub releases page) -> Publish to PyPI -> Publish to conda-forge

@JessicaS11
Copy link
Member

Thanks for all this info!

If I understand correctly, before, you made a git tag, pushed it to GitHub, and then published a release? You can also make a GitHub release directly on the GitHub releases page and this does both automatically in one click (tag and release).

I've done tags and releases both ways, and the triggering in Travis of PyPI builds hasn't always worked.

Every commit has a version. There are published versions like v0.3.0, and non-published interim versions like v0.3.0.dev1+abcdef (the 1st commit after v0.3.0).

So setuptools-scm looks for tagged commits? If a commit is not tagged, nothing happens. If the commit gets tagged (which must be done manually either through GitHub or git), then setuptools-scm will publish the version and in so doing figures out the updated version number (which hopefully also matches the release version number I've created; or does it get the version number from the tagged release?). Is it then setuptools-scm noticing a different tagged version or the tagged commit itself that triggers the publish to PyPI (and down the pipeline conda-forge)?

If there's no strong preference for using Travis CI, I can make a Publish to PyPI Github Actions workflow as I've got more experience on that. It will also publish test releases to TestPyPI so we can test things before doing the real release.

The Travis CI workflow also contains code for publishing test releases to TestPyPI. I looked for some info on switching to GitHub Actions versus staying with Travis, and it seems like the main pro Travis has is the ability to trigger a build manually through their API (and they have an API). That said, I'd like to keep Travis for testing for now but would be fine to have all of the builds+releases done through GitHub Actions. If you're still up for it @weiji14, I'd say go for making the change. I will work on getting the tokens set up with the same variable names they currently have for Travis.

@weiji14
Copy link
Member

weiji14 commented Feb 28, 2021

Ok, I've opened up #174 which migrates the PyPI deployment from Travis CI to Github Actions. We can do a few test commits there to show how the package publishing workflow works (to TestPyPI). Will need help setting up the PyPI tokens first though since I haven't got permissions.

@JessicaS11
Copy link
Member

JessicaS11 commented Mar 5, 2021

Success! Thanks @weiji14 for getting the GH Action to PyPI and TestPyPI (#174)set up! The first run (triggered by the merge, but I checked and the button to run it manually is available too) was successful.

I think that means we can go ahead with a new release to get everything on conda-forge, right? Does one of you want to submit the development --> master PR. Then the other two of us can review it and I'll tag it after the merge (or can I tag the would-be merge commit? I am still wondering how we make sure that my tag/release number and the automatic versioning with setuptools-scm match).

See first #175 to get the rest of the changes discussed here into development!

@weiji14
Copy link
Member

weiji14 commented Mar 8, 2021

A bit of a chicken and egg problem, but do we want to update the install instructions at https://icepyx.readthedocs.io/en/development/getting_started/install.html to mention conda install icepyx before the v0.3.3 release or after the release (i.e. in v0.4.0)?

@JessicaS11
Copy link
Member

It's funny you ask this because I almost updated the install instructions but then didn't for exactly the reason you noted. I think at this point we should do it after, once we've actually done a conda install icepyx ourselves. With the work Tian and others are putting through for visualization, it shouldn't be too long before v0.4.0.

@lsetiawan
Copy link
Contributor Author

Hey all, I've been MIA the past week, seems like you both made a lot of progress! Nice work! 😄. Is the geoviews stuff included in v0.3.3? If not, maybe we should move forward with that release, so we can get icepyx into conda, that way we are able to test it out.

@weiji14
Copy link
Member

weiji14 commented Mar 9, 2021

Hi Don, you're back just in time. We just merged in the conda brach at #175 so we'll need your help with the conda-forge package soon at conda-forge/staged-recipes#13866. Geoviews will have to wait for v0.4.0 as it's a bit complicated.

@JessicaS11 did you want to try and publish a release (following #170 (comment)) today? Let me know if you need any help or clarification.

@JessicaS11
Copy link
Member

Yeah, let's publish a release. @lsetiawan, can you submit a development--> master PR? Then @weiji14 and I can review it and I'll publish the release and we can check up on the conda-forge recipe.

@JessicaS11 JessicaS11 linked a pull request Mar 11, 2021 that will close this issue
@JessicaS11
Copy link
Member

I merged PR #179 and we've got a successful new release on PyPI. Our readthedocs builds are failing due to the switch to automatic versioning, and we now have the non-fast forward commit on master that's not on dev (I don't think there's an easy setting in GitHub to keep this from happening?).

@JessicaS11
Copy link
Member

To elaborate on the read the docs failure. When it tries to import icepyx during environment setup, it errors:

from _icepyx_version import version as __version__
ModuleNotFoundError: No module named '_icepyx_version'

Since _icepyx_version.py is created by setuptools_scm but not tracked by git, how do we get read the docs to create this file and get the version?

@weiji14
Copy link
Member

weiji14 commented Mar 11, 2021

Yikes, didn't see that one coming! I see readthedocs being mentioned at https://github.com/pypa/setuptools_scm#usage-from-sphinx, see if that helps? I'll take a look around some other projects who use readthedocs and setuptools_scm to see what their conf.py looks like.

@JessicaS11
Copy link
Member

Unfortunately the setuptools_scm guidance you linked above is no help (I tried that first - see the rtd_fix branch). If you put that code in there before import icepyx it can't find the version because the package isn't imported. If you put it in after, you get the ModuleNotFoundError.

Can one of you remind me if there was a particular reason we opted to use icepyx_version instead of the more stereotypical __version__? I haven't tried it yet, but I'm wondering if this will also be an issue for editable pip installs (as mentioned by @tsutterley in #148)?

@lsetiawan
Copy link
Contributor Author

lsetiawan commented Mar 11, 2021

Attempting fixing the read the docs at #181. My understanding is that read the docs will run an install, which would output the _icepyx_version.py and then the conf should read that.

@JessicaS11
Copy link
Member

Thanks @lsetiawan! Your fix is similar to the one of the ones I tried on rtd_fix, but has the same issue. conf.py contains import icepyx before looking for the version, and the failure is coming from the __init__.py file that contains from _icepyx_version import version as __version__. So it needs to be addressed before importing icepyx (which is chicken and egg with our solutions because they require the package to be imported already).

@weiji14
Copy link
Member

weiji14 commented Mar 11, 2021

I had a look at the conf.py file and I think we can remove the import icepyx line since it doesn't appear to be used in the script. Edit: nevermind, red herring.

@weiji14
Copy link
Member

weiji14 commented Mar 11, 2021

Readthedocs has a continuous Pull Request builder as of Jan 2021 (see https://blog.readthedocs.com/pull-request-builder-general-availability/). Should we maybe give that a go @JessicaS11 while we debug the situation? Will need your admin permissions to set it up according to https://docs.readthedocs.io/en/stable/pull-requests.html#preview-documentation-from-pull-requests.

@JessicaS11
Copy link
Member

JessicaS11 commented Mar 11, 2021

Readthedocs has a continuous Pull Request builder as of Jan 2021 (see https://blog.readthedocs.com/pull-request-builder-general-availability/).

Oh, fun! As admin I just do this manually, but it might be fun for new contributors to be able to see/test their docs updates without my setting them up. It looks like I just need to enable it from read the docs, so I will go ahead and do that. Update: enabled!

From the looks of it, either #181 or #182 solve the docs building issue. Thoughts or preferences for which one we merge in?

@weiji14
Copy link
Member

weiji14 commented Mar 11, 2021

Let's go with the one that works (i.e. #182). If we merge that one in to the 'development' branch, we ought to see the revision number change on https://icepyx.readthedocs.io/en/development/index.html (which is currently stuck on 9446737, i.e. the commit before #168).

image

Going forward, we should have a CI check that ensures the documentation is able to be built on readthedocs before pushing to the main branch.

@JessicaS11
Copy link
Member

Going forward, we should have a CI check that ensures the documentation is able to be built on readthedocs before pushing to the main branch.

Good idea. I'll take some blame for this one because I saw the docs building failures but didn't look at them too closely because I thought it was the same as the other intermediate issues we were having while we set up the automatic versioning.

@weiji14
Copy link
Member

weiji14 commented Mar 11, 2021

Going forward, we should have a CI check that ensures the documentation is able to be built on readthedocs before pushing to the main branch.

Good idea. I'll take some blame for this one because I saw the docs building failures but didn't look at them too closely because I thought it was the same as the other intermediate issues we were having while we set up the automatic versioning.

No no, I should be the one to blame since I suggested using setuptools_scm in the first place 😆. We all have to learn things the hard way sometimes, the problem is that things tend to break during releases for some reason (Murphy's Law).

@lsetiawan
Copy link
Contributor Author

No no, I should be the one to blame since I suggested using setuptools_scm in the first place laughing. We all have to learn things the hard way sometimes, the problem is that things tend to break during releases for some reason (Murphy's Law).

Best way to learn is experimenting with it! haha setuptools_scm is growing on me I might start using that in the future.

This has been a really great effort! Thank you both for helping out! I learned a lot!

@JessicaS11
Copy link
Member

Yes, it's been great to learn together! : )

@JessicaS11
Copy link
Member

So did we successfully build a recipe on conda-forge?

@weiji14
Copy link
Member

weiji14 commented Mar 11, 2021

Docs at https://icepyx.readthedocs.io/en/development/ point to 7458296 now, woohoo!

Looks like we might need a quick v0.3.4 release. How is everyone's schedule time-wise? I was thinking of pulling together a changelog for v0.3.3 too for https://icepyx.readthedocs.io/en/development/user_guide/changelog/index.html, but we could do it tomorrow instead.

So did we successfully build a recipe on conda-forge?

Yep, I can work on that with Don if you need to go since it's past 5pm on East Coast US?

@JessicaS11
Copy link
Member

Looks like we might need a quick v0.3.4 release. How is everyone's schedule time-wise? I was thinking of pulling together a changelog for v0.3.3 too for https://icepyx.readthedocs.io/en/development/user_guide/changelog/index.html, but we could do it tomorrow instead.

That sounds like a good plan. There should be a template in the changelog dir you linked to for creating the new one(s).

I have a couple other docs-related things I'd like to try and update quickly before 0.3.4, which I can hopefully get PRs in for tomorrow. You're right about me calling it a day for now!

@lsetiawan
Copy link
Contributor Author

When the conda-forge CI finishes, the recipe will live here: https://github.com/conda-forge/icepyx-feedstock

@lsetiawan
Copy link
Contributor Author

Time to try things out from conda-forge! https://anaconda.org/conda-forge/icepyx

@JessicaS11
Copy link
Member

I created a new dummy environment and installed icepyx via conda! Yay!

Thanks for all your hard work making this happen, @lsetiawan and @weiji14. What a great start to the weekend (or middle of it, as the case may be). : )

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.

3 participants