Skip to content

Build + Support Py39 and Various CI Updates #270

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

Merged
merged 21 commits into from
Jan 25, 2021
Merged

Build + Support Py39 and Various CI Updates #270

merged 21 commits into from
Jan 25, 2021

Conversation

evamaxfield
Copy link
Contributor

@evamaxfield evamaxfield commented Jan 24, 2021

I was originally trying to get numcodecs for py39 + macOS building as I have a build failing, but, in working on this:

In addressing the CI changes, I tried to figure out the travis and appveyor prior setups but it was a bit tricky. The one missing thing is coveralls which has a GitHub Action that we could use but didn't know how you wanted to handle that. (Sidenote: other option is to move to codecov which is very easy to setup in my experince)

Reference Issues:
#262
#261
#269

TODO:

  • Unit tests and/or doctests in docstrings
  • tox -e py39 passes locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • tox -e docs passes locally
  • Test coverage to 100% (Coveralls passes)

@joshmoore
Copy link
Member

Oh wow. Nice, @JacksonMaxfield. Briefly on the trade-off: @jakirkham suggests that moving to the upstream wheels will be doable and advantageous but will require a number of changes throughout the build system. Once done, it would mean that this repo could be python only. (But no one has yet started on that work) But it looks like you are close so worth moving forward?

If so, moving to codecov is completely valid and has been done elsewhere in the org. For that matter, I could see ignoring leaving it temporarily as a TODO as we figure out the other questions.

@evamaxfield
Copy link
Contributor Author

But it looks like you are close so worth moving forward?

Sorry, is this saying I should add the blosc wheel support to this PR or saying handling it in the future. I am a bit afraid to touch any of the c / pyx files not going to lie 😅

@@ -9,7 +9,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ubuntu-18.04, windows-latest, macos-latest]
os: [ubuntu-18.04, ubuntu-20.04, windows-latest, macos-latest]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary, or can we build wheel only on one of the two linux ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think both are needed. So I will just replace with ubuntu-latest.

@Carreau
Copy link
Contributor

Carreau commented Jan 25, 2021

I've disable making Appveyor mandatory if we move to gh-action so this can be merged even with appveyor failing. I'm +1 on moving to codecov as well.

@Carreau
Copy link
Contributor

Carreau commented Jan 25, 2021

Sorry, is this saying I should add the blosc wheel support to this PR or saying handling it in the future. I am a bit afraid to touch any of the c / pyx files not going to lie 😅

I think we can wait for the migration to the blosc wheel, i will likely not need to touch c/pyx beyond deleting them though, but might need minimal API change internally; I think there was a discussion to at least bump the minor version when we do that.

@joshmoore
Copy link
Member

Sorry, is this saying I should add the blosc wheel support to this PR or saying handling it in the future

The latter.

I think we can wait for the migration to the blosc wheel,

Exactly.

;) Sorry for being unclear.

@jakirkham
Copy link
Member

This looks really good! Thanks for working on this Jackson 😄

Did we want to add Windows to GitHub Actions as well?

@evamaxfield
Copy link
Contributor Author

Did we want to add Windows to GitHub Actions as well?

That should be pretty easy to do. Just didn't see any windows CI stuff in the repo besides the wheel building. I will add it in later today.

@evamaxfield
Copy link
Contributor Author

@jakirkham I spoke too soon. It seems that there is semi-known weird numpy + pickle interaction which results in a single one of the tests failing. Specifically the backwards compatibility test on pickles.

StackOverflow explaining the issue. I think it has to do with the "\r\n" / windows carrage return character.

Alright if I skip that test on Windows?

@jakirkham
Copy link
Member

Thanks Jackson! 😄

Yeah that sounds like a good idea. Maybe we can mark it as xfail on Windows? Could you please raise an issue as well so we can track fixing that in the future? 🙂

@evamaxfield
Copy link
Contributor Author

Yeah that sounds like a good idea. Maybe we can mark it as xfail on Windows? Could you please raise an issue as well so we can track fixing that in the future? slightly_smiling_face

Sounds good! Should be good to go w/ the latest Windows CI passing

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for adding that 🙂

Made a small suggestion to the xfail comment. Otherwise LGTM

@jakirkham
Copy link
Member

I've also checked the box to disable AppVeyor builds when appveyor.yml is missing. So hopefully that status check will disappear.

@jakirkham jakirkham merged commit f51502c into zarr-developers:master Jan 25, 2021
@jakirkham
Copy link
Member

Thanks again Jackson! 😄

@evamaxfield
Copy link
Contributor Author

Thank you all for making this package to begin with 🙂

@evamaxfield
Copy link
Contributor Author

Hey @jakirkham any chance to bumpversion and release to support py39?

@joshmoore
Copy link
Member

Tagged & pushed. 🤞🏼

@evamaxfield
Copy link
Contributor Author

Last update before I head to bed: my tests now pass w/ Mac Py39 support so hopefully not jinxing it when I say "seems like everything worked" 🙂

@jakirkham
Copy link
Member

Should now have wheels and Conda packages.

@joshmoore
Copy link
Member

@jakirkham : on that note, it looked like in docs/contributing.rst that I should take the checksum of the local .tar.gz?? Can you explicame a bit?

@jakirkham
Copy link
Member

Ah that's outdated. We can drop that. I think that was when we were still using GitHub archives instead of PyPI as the source of conda-forge packages

@jakirkham
Copy link
Member

Fixing the doc in PR ( #272 )

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.

4 participants