-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Py2F]: add profiling support & optimisations #449
Conversation
* Remove optimisations * Correctly pass sizes to bindings * Convert all granule stencils to CachedProgram
…o py2f-with-optimisations
Co-authored-by: Hannes Vogt <[email protected]>
…ix_roundtrip_import
…y2f-with-optimisations
cscs-ci run default |
.../diffusion/tests/diffusion_stencil_tests/test_temporary_fields_for_turbulence_diagnostics.py
Outdated
Show resolved
Hide resolved
@@ -43,3 +43,4 @@ variables: | |||
VIRTUALENV_SYSTEM_SITE_PACKAGES: 1 | |||
CSCS_NEEDED_DATA: icon4py | |||
TEST_DATA_PATH: "/project/d121/icon4py/ci/testdata" | |||
ICON_GRID_LOC: "/project/d121/icon4py/ci/testdata/grids/mch_ch_r04b09_dsl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be more future proof if you had here the ICON_GRID_PATH = "/project/d121/icon4py/ci/testdata/grids
and then extended the path in your test case? Then we could switch between grids more easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extending the path will currently not always work in ICON since the grids are not all in one root folder. For it to work we'd have to move all grids into one folder. For now I propose to keep it as is, which is the user can define ICON_GRID_LOC
which is the path to the root folder of the grid, and then can define ICON_GRID_NAME
which is the name of the grid (in each runscript), so already there is some flexibility here. Regarding the CI tests, I can improve that in my next PR which will be about py2fgen CI improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was only thinking about CI actually here... For me is fine to do this in a next PR.
model/atmosphere/diffusion/src/icon4py/model/atmosphere/diffusion/diffusion.py
Outdated
Show resolved
Hide resolved
@@ -69,3 +69,7 @@ def device(self): | |||
} | |||
device = device_map[self.icon4py_backend] | |||
return device | |||
|
|||
@cached_property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately ICON has a seperate namelist parameter for this, where I think that should be determined in the gridfile. In fact in the MCH grid file there is a global_grid = 0
property which might mean this... but it does not exist in the grid files provided by MPI-M, (for example the one we use for the Aquaplanet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not provided by MPI-M then there is no consistent way of passing it into the wrapper from ICON across experiments (MCH, APE). Therefore I would rather keep this environment variable for now, it is very easy to set in the run script, and doesn't require further changes on the ICON side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really care about the ICON side, what I am always a bit worried is that we constrain the pure python greenline runs to do awkward things because of ICON or the ICON integration. And all this grid file related things should eventually come from the model configuration file (same as ICON) and not from the environment. (This is different for the backend, I think it makes sense to run the exact same experiment (without changes in config) on different backends. ) Anyhow, since we don't have a consolidated configuration yet in icon4Py we leave it as is and rework it in due time. That might mean then that we would maybe also need to change things on how the py2fgen or the wrapper works...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, I agree we postpone it until we think about how to configure icon4py.
cscs-ci run default |
b4fa242
to
b4211d8
Compare
cscs-ci run default |
cscs-ci run default |
1 similar comment
cscs-ci run default |
launch jenkins spack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one small thing...
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
In case your change might affect downstream icon-exclaim, please consider running
For more detailed information please look at CI in the EXCLAIM universe. |
cscs-ci run default |
launch jenkins spack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to go from my side.
Adds CachedProgram to diffusion stencils, as well as other optimisations for py2f and changes so that APE experiments can be run with py2f.
Adds CachedProgram to diffusion stencils, as well as other optimisations for py2f and changes so that APE experiments can be run with py2f.
Description
Optimisations for diffusion granule as well as modifications to run both APE and MCH experiments.
CachedProgram
caches programs once compiled and reuses them across program calls. Skips lowering steps on each call.math.prod
in unpacking functions.isinstance
checks inCachedProgram
CachedProgram
and then passed along so they do not have to be extracted again each time. This feature can only be enabled once feat[next]: Optimisations for icon4py GridTools/gt4py#1536 is merged.