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

Bugfix sdist #953

Merged
merged 2 commits into from
Feb 27, 2020
Merged

Bugfix sdist #953

merged 2 commits into from
Feb 27, 2020

Conversation

timkpaine
Copy link
Member

@timkpaine timkpaine commented Feb 27, 2020

This should fix:

May also help fix:

Fixes (unrelated):

Don't know how possible it is to test this so i've added an extensive comment about the two library copies

@timkpaine timkpaine added bug Concrete, reproducible bugs Python labels Feb 27, 2020
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This is great - any suggestions on how to assert this behavior in a test in the future?

@texodus texodus merged commit b9b8059 into master Feb 27, 2020
@texodus texodus deleted the bugfix_sdist branch February 27, 2020 05:32
@ceball
Copy link
Contributor

ceball commented Feb 27, 2020

any suggestions on how to assert this behavior in a test in the future?

@texodus I think the overall best test of these kinds of things (in terms of coverage and practicality) is to actually install and test the package as end users would do. That sounds painful, but I would say that @timkpaine's conda package will already help a lot there.

At the risk of saying things everyone already knows...

The package build process (i.e. running just "conda build" once there is a recipe) results in:

  • creation of a clean (isolated) build environment
  • building of package in that build environment
  • creation of clean (isolated) installation environment
  • installation of package built above into clean installation environment
  • tests of that installed package (at least: does it import? but could be more, e.g. unit tests)

"Isolated" means as isolated as reasonably possible (but not docker levels of isolation).

The conda build process above will happen automatically on conda-forge (on all of the major platforms) whenever a new release of the package is uploaded to pypi - a PR at conda-forge will be opened automatically for the perspective package maintainers to review.

So that seems pretty good to me. Thanks Tim :)

@ceball
Copy link
Contributor

ceball commented Feb 27, 2020

I also have some related thoughts. Tacking them onto the end of a closed PR isn't the best idea, but I just happen to be here now.

The above process isn't perfect. E.g. it could result in having to upload a new release after putting out a bad one, which would be annoying. To avoid that, the above workflow could be done first with a dev or pre-release tag, i.e. pushing pre-releases (e.g. 0.1.2rc1 or whatever) to pypi and making sure they successfully go through the above process before pushing a release tag. Or, perspective's own CI could be set up to build the conda package locally from time to time.

It might also be worth considering having automated uploading to pypi from CI, e.g. based on a git tag.

Also, the above is mainly conda focused. Although the process will effectively be testing some parts of the python distribution and installation process, it's not a full test from the point of view of someone who will be doing "pip install perspective-python". However, the same kinds of ideas (e.g. testing what users will be installing) are covered in the python world via projects like tox (to the extent it's possible to cover "system level dependencies" with pip).

I'd be happy to help out with any of the above (will talk to Tim).

@texodus
Copy link
Member

texodus commented Feb 27, 2020

Is it possible to "install and test the package as a end users would do" without first disting it to conda-forge? I have no doubt this is the best way - but it doesn't prevent us from regressing this build support if we can only install & test the last version we disted?

@ceball
Copy link
Contributor

ceball commented Feb 27, 2020

Yes, it is. My comment was too long :)

perspective's own CI could be set up to build the conda package locally from time to time.

This is what most projects I personally work on do, rather than relying only on conda-forge. I.e. we keep the recipe (yaml file) in our github repo, and can then just run "conda build" whenever we want (locally as developers, and on CI).

We don't usually do it on every commit, just when we push a dev tag, becase generally the projects I'm talking about are pure python with quick test times, so the overhead of full package building is not worth it. But that might be different for perspective.

@timkpaine
Copy link
Member Author

We already do this for perspective, just only on one specific platform after doing an in-place build. Basically need to modify it to undo the in-place prior to running sdist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants