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: install mf6 development build for CI tests #1970

Closed
wants to merge 2 commits into from

Conversation

wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Sep 30, 2023

The motivation here is that flopy's develop branch should test against the mf6 development build so the projects can be developed in sync. The move to update flopy packages in CI was misguided — we want to test the codebase as it exists in version control.

Master and release branches are still tested against the latest official mf6 release.

* also, don't update flopy packages, test against current repository state
@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

Merging #1970 (f029c3d) into develop (4b105e8) will decrease coverage by 0.1%.
Report is 1 commits behind head on develop.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           develop   #1970     +/-   ##
=========================================
- Coverage     72.7%   72.7%   -0.1%     
=========================================
  Files          257     257             
  Lines        57810   57810             
=========================================
- Hits         42075   42057     -18     
- Misses       15735   15753     +18     

see 2 files with indirect coverage changes

@wpbonelli wpbonelli marked this pull request as ready for review September 30, 2023 14:06
Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

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

This is good. I guess we will need to stay on top of any changes that we make on the MODFLOW 6 side. If we keep the definition files in flopy, we could compare flopy dfns with modflow6 dfns to help identify why there might be a failure? Or maybe there is a better way?

@wpbonelli
Copy link
Member Author

wpbonelli commented Oct 2, 2023

After thinking on this more

The move to update flopy packages in CI was misguided — we want to test the codebase as it exists in version control.

the second part seems right in principle, but now I wonder if flopy needs to version package classes.

Flopy could cease versioning dfns and generated mf6 component classes, and instead package a copy of the latest dfns, and generate classes from it at install time. Flopy CI could fetch and test both the latest released dfns and the develop version. The flopy develop branch and releases could aim always to support (at minimum) the latest mf6 release and the develop version. Compatibility with older mf6 would need to be tested and if found lacking a recommendation to downgrade to a previous flopy seems appropriate?

This could help to make mf6 version support policy explicit and limit overhead to keep flopy and mf6 in sync. Autotests could test multiple mf6 versions with fairly little extra effort

It seems like this would not disturb the user experience — API reference would still be on ReadTheDocs, package classes would still be in the environment after installation, and it shouldn't add much time to the install process.

Some possible downsides:

  • would intellisense still work?
  • would this make debugging harder?

@wpbonelli
Copy link
Member Author

wpbonelli commented Oct 3, 2023

I think MF6 class generation could be deferred to install time with a custom setuptools build command by extending setuptools.command.build.SubCommand. The second paragraph here makes it sound like this is not uncommon

A command that produces platform-independent files (e.g. compiling text templates into Python functions)

@wpbonelli wpbonelli marked this pull request as draft October 6, 2023 16:17
@wpbonelli wpbonelli closed this Mar 27, 2024
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.

2 participants