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

refactor(createpackages): use jinja for mf6 module code generation #2333

Draft
wants to merge 75 commits into
base: develop
Choose a base branch
from

Conversation

wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Oct 10, 2024

Carved out of #2317 — just move to Jinja as an initial step, type hints can come afterwards as they will take some effort to get right. The aim here is to match the old createpackages.py capabilities without relying on mfstructure.py. This is an initial step to move mfstructure.py to a leaner representation of the input spec. mfstructure.py is still used at runtime which will need to be unraveled in followup work.

Introduce a flopy.mf6.utils.codegen module with some new machinery for loading an input specification from DFN files into an intermediate representation, and generating source code with Jinja. This is fairly general, handling quirks of the existing framework in a "shim" of Jinja filters that transform the template context before rendering. These can be removed as we rework things. The templates should also get simpler over time.

For now the load routine uses a data structure from the boltons library to represent a DFN as an unstructured (flat) map of variables before structural parsing, where variables can have duplicate names. If we could guarantee all variable names were unique by changing a few DFNs this wouldn't be needed. In any case it will not be necessary once we have structured TOML definition files.

Jinja2, boltons, tomlkit and modflow-devtools are added to a new optional dependency group codegen. These are required to use the code generation utilities — this may require CI tweaks in related repositories and should be noted by developers too.

Miscellaneous:

Tentative next steps after this PR:

  1. Refactor input spec introspection

    • store the new structured DFN representation on component classes instead of the original unstructured DFN
    • once this is in place, we can simplify mfstructure.py to inspect this instead of parsing lists of strings
  2. Start using structured DFN files. We have discussed prototyping this for the time being in a way that is not visible to the user, and refining the format until ready to adopt upstream in MF6. A first draft TOML conversion script has been added — we could later convert DFNs -> TOML at code generation time, then exercise the TOML load.

  3. Explicit versioning for the DFN specification language. TOML could be considered v2 where V1 is the original DFN format.

  4. Add type hints as per feature: Adding Python type hints for input specifications #2298

  5. Drop *TemplateGenerator mechanism if possible

@wpbonelli wpbonelli force-pushed the jinja branch 2 times, most recently from a3b7c2e to af1e7a5 Compare October 10, 2024 17:49
@wpbonelli wpbonelli added modflow 6 dependencies Pull requests that update a dependency file maintenance labels Oct 10, 2024
@wpbonelli wpbonelli force-pushed the jinja branch 4 times, most recently from d1e8a21 to a7f2644 Compare October 10, 2024 18:30
@wpbonelli wpbonelli added this to the 3.9 milestone Oct 10, 2024
Copy link
Contributor

@deltamarnix deltamarnix left a comment

Choose a reason for hiding this comment

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

I haven't looked through everything, but I left you a PR on your own branch with indications on how I would probably approach this. Let's take a look at it together and see if we can get rid of the shim :)

autotest/test_codegen.py Outdated Show resolved Hide resolved
docs/mf6_dev_guide.md Outdated Show resolved Hide resolved
docs/mf6_dev_guide.md Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
flopy/mf6/data/dfn/utl-tas.dfn Show resolved Hide resolved
flopy/mf6/utils/codegen/templates/context.py.jinja Outdated Show resolved Hide resolved
flopy/mf6/utils/codegen/shim.py Outdated Show resolved Hide resolved
@wpbonelli wpbonelli force-pushed the jinja branch 2 times, most recently from bab83ac to cf68432 Compare October 18, 2024 00:32
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 90.74675% with 57 lines in your changes missing coverage. Please review.

Project coverage is 76.5%. Comparing base (bb9824e) to head (9a25689).
Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
flopy/mf6/utils/codegen/dfn2toml.py 0.0% 30 Missing ⚠️
flopy/mf6/utils/codegen/filters.py 93.5% 15 Missing ⚠️
flopy/mf6/utils/codegen/dfn.py 96.0% 10 Missing ⚠️
flopy/mf6/utils/createpackages.py 75.0% 1 Missing ⚠️
flopy/mf6/utils/generate_classes.py 83.3% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #2333     +/-   ##
=========================================
+ Coverage     68.4%   76.5%   +8.0%     
=========================================
  Files          294     300      +6     
  Lines        59390   63072   +3682     
=========================================
+ Hits         40652   48261   +7609     
+ Misses       18738   14811   -3927     
Files with missing lines Coverage Δ
flopy/mf6/data/mfdatastorage.py 75.4% <ø> (+6.5%) ⬆️
flopy/mf6/utils/codegen/__init__.py 100.0% <100.0%> (ø)
flopy/mf6/utils/codegen/context.py 100.0% <100.0%> (ø)
flopy/mf6/utils/createpackages.py 80.0% <75.0%> (+73.5%) ⬆️
flopy/mf6/utils/generate_classes.py 20.3% <83.3%> (+2.5%) ⬆️
flopy/mf6/utils/codegen/dfn.py 96.0% <96.0%> (ø)
flopy/mf6/utils/codegen/filters.py 93.5% <93.5%> (ø)
flopy/mf6/utils/codegen/dfn2toml.py 0.0% <0.0%> (ø)

... and 226 files with indirect coverage changes

@wpbonelli
Copy link
Member Author

wpbonelli commented Dec 15, 2024

After MODFLOW-USGS/modflow-devtools#167 this could now be reworked to run the toml conversion script, load toml directly into jinja contexts, and dispense with dfn.py, which would make the PR much smaller. The conversion script is an adapter which we can eventually get rid of also, once DFN files go to TOML. (And at that point we might also want to consider eliminating devtools from the process, if we don't mind duplicating some http client boilerplate to download and unzip the dfns?)

In any case this will have to wait for 3.10 as 3.9 is imminent

@wpbonelli wpbonelli marked this pull request as draft December 15, 2024 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file maintenance modflow 6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants