mdbartos / pysheds

:earth_americas: Simple and fast watershed delineation in python.
GNU General Public License v3.0
702 stars 188 forks source link

Packaging improvements, Use pytest fixtures, Support Python3.11 #220

Closed Zeitsperre closed 1 year ago

Zeitsperre commented 1 year ago

Hi,

I was looking at what your package needed in order to support Python3.11, and I realized there were a few things that could be improved. There's a lot going on here, so here's a summary:

Changes

I was having issues with pytest segfaulting as well, so l I also made some significant changes to how pytest tests are configured and run.

Previously, the expectation was that tests are run in sequence, with some tests mutating a base class used further down. This design made it so that all tests are co-dependent, going against that design principles of pytest.

The approach I've done here is to create a few base objects that are instantiated on every test run when they are called (not ideal and can very likely probably be optimized). What this does is make it so that test failures in some tests should not impact subsequent tests. For tests that create temporary files, I've applied the tmp_path fixture so that they do not create artifacts that need to be removed by the user.

My impression from all these changes is that pytest is much quicker to load and no longer segfaults on my local machine.

For more information about pytest fixtures: https://docs.pytest.org/en/7.1.x/explanation/fixtures.html#about-fixtures

I didn't want to go overboard, but if you would appreciate further changes, such as more uniform numpy-docstring conventions, I don't mind continuing with this work in this PR or in another.

If you'd like any clarifications, please let me know.

Best,

mdbartos commented 1 year ago

Thank you for submitting this PR. I should be able to review this weekend.

mdbartos commented 1 year ago

Thanks for taking the time to write this PR @Zeitsperre. I am not quite sure why the tests are not running with GitHub actions in this PR. Any idea why?

Zeitsperre commented 1 year ago

Hi there!

Please excuse the tardiness (I was on vacation). I think there must be a setting in the repo that prevents non-maintainers from being able to run GitHub Actions. Maybe check the permissions?

See also: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository

mdbartos commented 1 year ago

Ok, that last commit seemed to do it.

Zeitsperre commented 1 year ago

Great! I'll fix my merge conflicts tomorrow (I'm still technically on vacation). Best!

mdbartos commented 1 year ago

Hi @Zeitsperre, can you update your summary to reflect the commits added after the PR was initiated? Thanks.

Zeitsperre commented 1 year ago

Hi @mdbartos,

I've updated the description. My last commit undoes the previous commits to try and get the development version of llvmlite (a numba dependency) working. I'm not sure if you squash commits, but if you'd like me to try dropping those commits manually, I can do that.

IMO this PR is ready for proper review. Thanks!

mdbartos commented 1 year ago

Quick request: can you change the version number in pysheds/init.py to 0.3.4?

Thanks, MDB

mdbartos commented 1 year ago

Also, I think the status badges on the readme (and github pages) are outdated. If you could fix with this PR, it would be much appreciated.

Zeitsperre commented 1 year ago

Badge is fixed and the version number seems to be already at v0.3.4!

For the install recipes, I went with dev and recipes (to reflect the requirements needed to run the notebooks under "recipes"). Are these names to your liking?

mdbartos commented 1 year ago

Looks good to me! Ready to merge when you give the go-ahead.

Zeitsperre commented 1 year ago

Great! Yes, LGTM!