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

CI: Use pip and setuptools to install pycalphad instead of Anaconda #331

Merged
merged 20 commits into from May 22, 2021

Conversation

bocklund
Copy link
Collaborator

@bocklund bocklund commented May 22, 2021

The new minimizer (#329) allowed us to drop IPOPT as a solver. All our remaining package dependencies are pure Python or have wheels shipped on PyPI (specifically, SymEngine), so installing all our dependencies from pip is now possible.

This PR:

  • Changes the Github Actions scripts to use PyPA/build to build a wheel and pip to install the package
  • Packages the pycalphad.tests package
  • Uses the pytest flags --import-mode=append and --pyargs pycalphad flags to ensure that we test the installed version of the package and not the local version in the working directory.

I (accidentally) verified that we test the installed package because pytest didn't discover the tests until I packaged them in setup.py.

Things to consider:

  1. Should we add pytest (and pytest-cov?) as runtime dependencies so users can reproduce tests locally without installing extra packages?
  2. We could think about introducing something like tox to automate this process and make it easier to build and test the installed version locally. That is probably overengineering it - I don't think we've hit any issues yet to justify our need, but it is an alternative.

@codecov
Copy link

codecov bot commented May 22, 2021

Codecov Report

Merging #331 (bcdc9db) into develop (391b222) will decrease coverage by 2.82%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #331      +/-   ##
===========================================
- Coverage    87.34%   84.52%   -2.83%     
===========================================
  Files           46       46              
  Lines         4521     4524       +3     
===========================================
- Hits          3949     3824     -125     
- Misses         572      700     +128     
Impacted Files Coverage Δ
pycalphad/_version.py 1.77% <0.00%> (-44.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 391b222...bcdc9db. Read the comment docs.

@richardotis
Copy link
Collaborator

  1. Is pytest already pulled in at runtime by one of our dependencies?
  2. tox is nice as a development tool but I don't think it meets a need for local user testing. tox wants to run everything in an isolated environment, but the point of running tests locally is that they run in the user's actual environment. It shouldn't make a difference, but it's the cases where it does where this can actually help diagnose the issue.

@bocklund
Copy link
Collaborator Author

Switching to testing the installed package is breaking coverage. Need to check whether pytest-cov supports coverage of installed versions

@bocklund
Copy link
Collaborator Author

  1. Is pytest already pulled in at runtime by one of our dependencies? - @richardotis

It doesn't look like it. We run a pip list before installing pytest and pytest-cov in the CI and it's neither are installed.

@bocklund
Copy link
Collaborator Author

For coverage, the code getting covered are in site-packages now, but coverage is measuring the lines hit in the version of pycalphad in the current working directory.

Our options are

1) Pass the site package path to coverage

coverage has support for this in that we need to specify the [paths] and point to the site-packages so it will treat site-packages/pycalphad as eqivalent to ./pycalphad. See this blog for some explanation (after the * * * in the Combined Coverage section). Here's an example of what a .coveragerc would look like. We'd just need the [paths] part.

However, the tricky aspect is that both these examples assume some kind of local virtual environment where the site packages directory can be found within the local directory. The way it's set up, it's installed to wherever on the machine (platform dependent) the site packages are installed. Since I don't think it's possible to be able to set that path for any machine from a global config file, we need to have some kind of local environment, so either a tool like tox or setting up a virtual environment using one of the many tools out there.

Note that this method still allows for coverage to be run on the local ./pycalphad package.

2) Test the local package instead of or in addition to the installed version

Maybe we just add another CI task specfically for coverage that uses a smaller build matrix and tests against the local version? We'd just need to decide which platforms and versions of Python to test.


My preference is for option 1) and setting up a virtual environment using the built-in venv because this is likely how we will reccomend users create development environments.

@bocklund
Copy link
Collaborator Author

bocklund commented May 22, 2021

coverage xml --include='.,*/site-packages' may also work from the command line if we don't want to introduce a .coveragerc.

I think we can use pyproject.toml and/or setup.cfg so no new file.

@bocklund
Copy link
Collaborator Author

I ended up going the virtual environment route and put the requiste coverage.py settings in pyproject.toml (also moved our pytest settings over from setup.cfg).

Seems like we no longer cover _version.py, but everything else is looking good.

Do we want to make a decision here on depending on pytest/pytest-cov at runtime?

@bocklund bocklund requested a review from richardotis May 22, 2021 18:03
@richardotis
Copy link
Collaborator

I think depending on pytest and pytest-cov is fine.

@richardotis
Copy link
Collaborator

An alternative would be to use the extras feature to do something like pip install pycalphad[tests], but then it has to be maintained on conda too.

@bocklund bocklund merged commit c150965 into pycalphad:develop May 22, 2021
@bocklund bocklund deleted the CI-setuptools-builds branch May 22, 2021 20:07
@richardotis richardotis mentioned this pull request May 24, 2021
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.

None yet

2 participants