Updating submodules and pace refactor #75

fmalatino · 2024-03-21T19:32:07Z

Description
This PR updates the NDSL, pyFV3, and pySHiELD submodules to most recent commits, reflecting the standardized import methods.

How Has This Been Tested?
Tested using the currently implemented unit tests in tests/main

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included

…s.txt

… methods

oelbert

Two quick questions on the driver init and relative imports, just because it looks a little awkward but nothing blocking

oelbert · 2024-04-01T15:55:51Z

driver/pace/driver/__init__.py

+    MonitorDiagnostics,
+    NullDiagnostics,
+    ZSelect,


What's the logic behind adding these here?

I was just copying what was done in NDSL, with exposing without rationale, and then un-exposing when it is deemed unnecessary.

oelbert · 2024-04-01T18:01:17Z

driver/pace/driver/run.py

@@ -6,8 +6,7 @@
 import yaml

 from ndsl.logging import AVAILABLE_LOG_LEVELS, ndsl_log
-
-from .driver import Driver, DriverConfig
+from pace.driver.driver import Driver, DriverConfig


What's the reason for this change?

I am following the style that was set forth in how I handled imports in the submodules. When I am within a package and I am referencing something internal to the package, the absolute path is used, when outside the package the relative path is used.

Yeah that makes sense, it just looks funny here

oelbert

👍

bensonr

As I look at the driver/pace/driver code I have a question about what we are building with Pace. If Pace is designed to be the model and we will be adding other components (land, ice, ocean, waves, etc.) within, I'm not sure it makes senes to treat the driver as a submodule. However, if we envision Pace being say the atmospheric component within a larger module, it does make sense to leave the submodule treatment in place.

fmalatino · 2024-04-09T18:33:46Z

As I look at the driver/pace/driver code I have a question about what we are building with Pace. If Pace is designed to be the model and we will be adding other components (land, ice, ocean, waves, etc.) within, I'm not sure it makes senes to treat the driver as a submodule. However, if we envision Pace being say the atmospheric component within a larger module, it does make sense to leave the submodule treatment in place.

@oelbert ?

oelbert · 2024-04-09T21:00:57Z

My understanding has been that Pace is solely an atmospheric model and any ocean or land model would be separate

lharris4 · 2024-04-09T21:23:13Z

Currently Pace is intended to be an atmosphere model, possibly containing a land component. Ocean and ice are separate from Pace's scope. Lucas

…

On Tue, Apr 9, 2024 at 5:01 PM Oliver Elbert ***@***.***> wrote: My understanding has been that Pace is solely an atmospheric model and any ocean or land model would be separate — Reply to this email directly, view it on GitHub <#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMUQRVBBT3NTRO5LWX5ZW5DY4RJKDAVCNFSM6AAAAABFCCM2UOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBWGAZTSOBTGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

bensonr

We are using srun in the .jenkins code. Should we use a macro that can be either srun or mpirun as we shouldn't always assume a slurm environment?

Is all of the documentation that is NDSL, pyFV3, or pySHiELD specific also duplicated in those repos? Or better yet, it should live in those repos and become available via the submodule includes.

The driver/pace structure still seems redundant to me. Perhaps it is my lack of Python semantics, but we are in the Pace repo and have a driver/. Why do I need to yet again remind you this is pace by having driver/pace/? Is it better to have pace/ or pace/driver so that all references are pace.X or pace.driver.X?

bensonr · 2024-04-16T11:38:15Z

Makefile

@@ -142,8 +142,8 @@ test_mpi_54rank:
 	mpirun -n 54 $(MPIRUN_ARGS) python3 -m mpi4py -m pytest tests/mpi_54rank


Will we always be using mpirun here or is there a chance it could be another invoker?

A change here would be prompted by a change in the docker image

bensonr · 2024-04-16T12:09:43Z

driver/pace/README.md


 This package provides command-line routines to run the Pace model, and utilities to write model driver scripts.

-We suggest reading the code in the examples directory, or taking a look at `pace/driver/run.py` to see how the main entrypoint for this package works.
+We suggest reading the code in the examples directory, or taking a look at `driver/pace/driver/run.py` to see how the main entrypoint for this package works.


Did the structure change to driver/pace/run.py or is it still driver/pace/driver/run.py?

oelbert · 2024-04-16T18:40:26Z

The driver/pace structure still seems redundant to me. Perhaps it is my lack of Python semantics, but we are in the Pace repo and have a driver/. Why do I need to yet again remind you this is pace by having driver/pace/? Is it better to have pace/ or pace/driver so that all references are pace.X or pace.driver.X?

pace/driver/pace/driver/* structure is definitely a relic of Pace's development history, and we want to resolve that. Do you think we should do it here or in a separate PR?

bensonr · 2024-04-16T19:14:54Z

In response to this comment, this PR is already renaming things from driver/pace/driver. Instead of doing it yet again, I was hoping we could get the structure set in one step. As always, I want the approach with the least chance of breaking things.

fmalatino · 2024-04-16T21:14:24Z

In response to this comment, this PR is already renaming things from driver/pace/driver. Instead of doing it yet again, I was hoping we could get the structure set in one step. As always, I want the approach with the least chance of breaking things.

Currently the method of setting up the pace package differs from its submodules in that it uses a requirements file for specifying the requirements for the pip install of the package, as opposed to passing them through a setup.py or other build backend method. Within the requirements file we specify a need for an editable install of the submodules and pace, which will search for a setup.py file in each of the directories specified. Each of the setup.py files look for a namespace package matching the specified name(s) in the call to the find_namespace_packages. These namespace packages must be contained within a directory alongside the setup.py file.
Proposed in PR:

driver/
├── pace/      
│   ├── __init__.py      
│   └── driver.py
│   └── other.py            
└── setup.py

Current:

driver/
├── pace/ 
│     ├── driver/      
│          ├── __init__.py      
│          └── driver.py
│          └── other.py            
└── setup.py

I chose driver to serve as the outer directory to contain the pace package and the setup.py with the intention that once installed the user would then use the package like:

import pace

driver = pace.Driver(...)

As opposed to what existed previously:

import pace.driver

driver = pace.driver.Driver(...)

We can change the use of setup.py to include all of the desired requirements and make calls to install the submodules as well, which will then treat the cloned repo as the outer directory containing the pace package alongside its setup.py:

cloned_pace/
├── pace/      
│   ├── __init__.py      
│   └── driver.py
│   └── other.py            
└── setup.py
└── other_mods/

This will however not use the requirements file method, and will require some minor changes elsewhere, like in the github workflows, etc. I have tested both of these methods and went with the former as it keeps prevents a need to change the other files. If the latter is preferred the changes can be easily made. Also, I am not attached to the name driver for the outer directory, only using it as it made it easy to implement.

fmalatino · 2024-04-17T14:19:31Z

@bensonr and @oelbert

I have made changes to the requirements_dev.txt file which should allow for pace to be installed without an outer directory (aside from it being contained in the cloned repo).

bensonr

Thanks for taking my suggestions regarding structure. You've done a great job making it work. There are two minor nits that could be saved for a future issue/PR.

bensonr · 2024-04-17T16:43:27Z

pace/README.md


 This package provides command-line routines to run the Pace model, and utilities to write model driver scripts.

-We suggest reading the code in the examples directory, or taking a look at `pace/driver/run.py` to see how the main entrypoint for this package works.
+We suggest reading the code in the examples directory, or taking a look at `driver/pace/driver/run.py` to see how the main entrypoint for this package works.


I think this should be pace/run.py.

Amended in 15fc295

bensonr · 2024-04-17T16:54:42Z

setup.py

-    setup_requires=setup_requirements,
-    tests_require=test_requirements,
-    name="pace-driver",
+    name="pace",


Do we need to update more of this info here in the setup block here?

I think this may be a good issue as I am not sure what would be appropriate to retain relating to the AI2 work.

oelbert

Phew, that's a lot of changes!

mlee03 · 2024-04-19T14:24:38Z

.github/workflows/main_unit_tests.yml

@@ -0,0 +1,31 @@
+name: "Main unit tests"


Is this workflow file needed?

Are the tests contained within that directory also tested in CI? If so, it can be removed.

fmalatino added 11 commits February 5, 2024 15:36

Updated submodule versions

00cba46

Loosened constraint on gridtools-cpp for build of gt4py in constraint…

3bbe036

…s.txt

Merge branch 'main' into refactor/submodule_updates

c250df0

Updating submodules to most recent versions as of 6 Feb 2024 16:00

d5bfeb0

Merge branch 'develop' into refactor/submodule_updates

16b15ce

Updated submodules

82c4862

Updated submodules to current commits in develop branches, and import…

ecda385

… methods

Updating pyFV3 submodule

6a7d6c3

Updated constraints.txt

35c4ed9

Removed local references in constraints.txt

63a46ac

Removed local reference to dace in constraints.txt

917b9c8

fmalatino requested review from oelbert and bensonr March 22, 2024 19:42

Updated pace internal imports

09045d1

fmalatino marked this pull request as ready for review March 25, 2024 15:27

oelbert reviewed Apr 1, 2024

View reviewed changes

Unexposed modules in driver __init__ and updated NDSL

4e2e3f7

fmalatino requested a review from oelbert April 2, 2024 13:50

oelbert approved these changes Apr 2, 2024

View reviewed changes

Updating to latest version of develop

2ba7691

bensonr reviewed Apr 9, 2024

View reviewed changes

Linting

7bda2c6

fmalatino added 5 commits April 15, 2024 14:12

pace refactor

5e910e2

update to current pyFV3

ae07176

Merged in current develop branch

5f83fde

Changing location of setup.cfg

4bb8877

Removed need for python path to be re-passed

3cb419e

Linting

8fb03f2

fmalatino requested review from bensonr and oelbert April 15, 2024 23:02

Updates to reflect new structure

f2724ce

bensonr reviewed Apr 16, 2024

View reviewed changes

Edits to requirements_dev.txt to install editable pace package

ec1e2e3

fmalatino requested a review from bensonr April 17, 2024 14:07

bensonr approved these changes Apr 17, 2024

View reviewed changes

Updated README

15fc295

oelbert approved these changes Apr 17, 2024

View reviewed changes

fmalatino changed the title ~~Updating submodules and import methods~~ Updating submodules and pace refactor Apr 18, 2024

fmalatino added 2 commits April 18, 2024 08:24

Updating submodules after NDSL 2024.04.00 release

e154802

Updating constraints.txt

4131fba

fmalatino merged commit 7e435fa into develop Apr 18, 2024
4 checks passed

mlee03 reviewed Apr 19, 2024

View reviewed changes

fmalatino deleted the refactor/submodule_updates branch June 25, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating submodules and pace refactor #75

Updating submodules and pace refactor #75

fmalatino commented Mar 21, 2024 •

edited

Loading

oelbert left a comment

oelbert Apr 1, 2024

fmalatino Apr 1, 2024

oelbert Apr 1, 2024

fmalatino Apr 1, 2024

oelbert Apr 1, 2024

oelbert left a comment

bensonr left a comment

fmalatino commented Apr 9, 2024

oelbert commented Apr 9, 2024

lharris4 commented Apr 9, 2024 via email

bensonr left a comment

bensonr Apr 16, 2024

oelbert Apr 17, 2024

bensonr Apr 16, 2024

oelbert commented Apr 16, 2024

bensonr commented Apr 16, 2024

fmalatino commented Apr 16, 2024

fmalatino commented Apr 17, 2024

bensonr left a comment

bensonr Apr 17, 2024

fmalatino Apr 17, 2024

bensonr Apr 17, 2024

fmalatino Apr 17, 2024

oelbert left a comment

mlee03 Apr 19, 2024

fmalatino Apr 19, 2024

		@@ -142,8 +142,8 @@ test_mpi_54rank:
		mpirun -n 54 $(MPIRUN_ARGS) python3 -m mpi4py -m pytest tests/mpi_54rank

Updating submodules and pace refactor #75

Updating submodules and pace refactor #75

Conversation

fmalatino commented Mar 21, 2024 • edited Loading

oelbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oelbert left a comment

Choose a reason for hiding this comment

bensonr left a comment

Choose a reason for hiding this comment

fmalatino commented Apr 9, 2024

oelbert commented Apr 9, 2024

lharris4 commented Apr 9, 2024 via email

bensonr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oelbert commented Apr 16, 2024

bensonr commented Apr 16, 2024

fmalatino commented Apr 16, 2024

fmalatino commented Apr 17, 2024

bensonr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oelbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmalatino commented Mar 21, 2024 •

edited

Loading