Adding and updating init files for modules #14

fmalatino · 2024-02-14T17:44:08Z

Description
Adding init files to standardize import method of modules

How Has This Been Tested?
Tested via lint and currently implemented translate tests
OS: RHEL 8.9 Ootpa
Built conda environment using workflow in github action configurations

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

FlorianDeconinck

Missing initialization imports (QuantityFactory is a big one)

ndsl/__init__.py

FlorianDeconinck · 2024-02-16T16:33:59Z

Please merge in develop instead of main

oelbert

I'd like to see typing exposed at a higher level, and a couple of questions about the layout otherwise.

ndsl/comm/__init__.py

ndsl/dsl/dace/__init__.py

ndsl/dsl/__init__.py

ndsl/__init__.py

FlorianDeconinck

Good job so far.

I am still debating internally if I dont' want to see two tier of exposure. One first tier of features we almost always need e.g. (Comm, Orchestration, Halo, etc.) and a second tier of more uncommon feature e.g. (CompareToNumpyStencil, Snapshot, ZarrMonitor, etc.)

ndsl/dsl/dace/__init__.py

tests/dsl/__init__.py

…iELD

oelbert

My main request is that I don't love taking so much out of the init files, and would prefer we kept more in them. If that causes problems with circular imports I'd rather we prioritize the NDSL interface over removing relative imports internally, though I believe there should be other workarounds

ndsl/__init__.py

ndsl/checkpointer/__init__.py

oelbert · 2024-02-27T20:35:00Z

ndsl/__init__.py

+    TilePartitioner,
+)
+from .dsl import (
+    CompareToNumpyStencil,


Not sure if we need this at the top level

This is advanced system but for now it's the only one, so I'd leave it here

ndsl/dsl/__init__.py

ndsl/grid/__init__.py

oelbert · 2024-02-27T21:11:40Z

ndsl/performance/__init__.py

@@ -1,2 +1,2 @@
-from .config import PerformanceConfig


I would also leave this in as well

Just leave in PerformanceConfig or/and NullTimer, Timer?

I'd say the tree are useful here either for us or typing

Left all three in.

ndsl/stencils/__init__.py

tests/dsl/__init__.py

tests/quantity/__init__.py

ndsl/__init__.py

oelbert

Getting there! I think there's more that can be moved down the chain, but my main quibble is with defining abstract base classes in ndsl.typing instead of keeping them where they were and importing them into ndsl.typing for exposure

ndsl/checkpointer/base.py

ndsl/__init__.py

oelbert · 2024-03-06T16:39:19Z

ndsl/typing.py

I think the idea here is not so much that typing would define a Communicator or Checkpointer base class but that they would be imported into typing so that they would be exposed here when you import ndsl.typing

ndsl/comm/communicator.py

oelbert · 2024-03-06T16:41:21Z

ndsl/comm/partitioner.py

the Partitioner abc should still live here IMO

oelbert · 2024-03-06T16:44:44Z

ndsl/grid/__init__.py

-    xyz_to_lon_lat,
-)
+from .eta import HybridPressureCoefficients
+from .generation import GridDefinition, GridDefinitions, MetricTerms


GridDefinition doesn't need to be exposed IMO

oelbert · 2024-03-06T16:48:22Z

ndsl/stencils/__init__.py

+from .testing.grid import Grid  # type: ignore
+from .testing.parallel_translate import (
+    ParallelTranslate,
+    ParallelTranslate2Py,
+    ParallelTranslate2PyState,
+    ParallelTranslateBaseSlicing,
+    ParallelTranslateGrid,
+)
+from .testing.savepoint import SavepointCase, Translate, dataset_to_dict
+from .testing.temporaries import assert_same_temporaries, copy_temporaries
+from .testing.translate import (
+    TranslateFortranData2Py,
+    TranslateGrid,
+    pad_field_in_j,
+    read_serialized_data,
+)


I think the testing stuff could still be in stencils.testing but that's not a big deal either way

Move down to testing it's specific enough

FlorianDeconinck · 2024-03-06T17:02:40Z

Getting there! I think there's more that can be moved down the chain, but my main quibble is with defining abstract base classes in ndsl.typing instead of keeping them where they were and importing them into ndsl.typing for exposure

Yes ndsl.typing should be a stub, code should remain in the original places.

E.g.

typing.py

from ndsl.comm import Communicator

any calling code can then do

from ndsl.typing import Communicator

It is more correct and also would allow us to swap implementation below without touching the exposure

FlorianDeconinck

We are getting there. This raises a ton of good question on the architecture of this monster. Very necessary work

FlorianDeconinck · 2024-03-07T14:52:01Z

ndsl/checkpointer/__init__.py

 from .null import NullCheckpointer
-from .snapshots import SnapshotCheckpointer
+from .snapshots import SnapshotCheckpointer, _Snapshots


Do we need _Snapshots? We should always try to have _X class or functions hidden, as this what the prefixed _ is suppose to hint at

ndsl/comm/__init__.py

ndsl/comm/communicator.py

ndsl/dsl/stencil.py

setup.py

tests/dsl/test_stencil_config.py

tests/dsl/test_stencil_wrapper.py

tests/dsl/__init__.py

tests/checkpointer/__init__.py

tests/quantity/__init__.py

FlorianDeconinck

A few notes to addressed that I poked

FlorianDeconinck · 2024-03-07T18:18:47Z

ndsl/__init__.py

+from .types import Allocator, AsyncRequest, NumpyModule
+from .units import UnitsError


FlorianDeconinck · 2024-03-07T18:21:09Z

ndsl/monitor/__init__.py

@@ -1,3 +1,2 @@
 from .netcdf_monitor import NetCDFMonitor
-from .protocol import Monitor


FlorianDeconinck · 2024-03-07T18:21:13Z

ndsl/performance/__init__.py

@@ -1,2 +1,2 @@
-from .config import PerformanceConfig


setup.py

FlorianDeconinck

LGTM

oelbert

1 minor requests that isn't blocking. Nice job

oelbert · 2024-03-08T18:24:30Z

ndsl/monitor/__init__.py

@@ -1,3 +1,2 @@
 from .netcdf_monitor import NetCDFMonitor
-from .protocol import Monitor


Some day ZarrMonitor should be as well, once zarr is more prevalent (fingers crossed)

oelbert · 2024-03-08T18:33:17Z

ndsl/typing.py

+from ndsl.comm.local_comm import AsyncResult, ConcurrencyError
+from ndsl.comm.null_comm import NullAsyncResult
+from ndsl.comm.partitioner import Partitioner
+from ndsl.performance.collector import AbstractPerformanceCollector
+from ndsl.types import AsyncRequest, NumpyModule
+from ndsl.units import UnitsError


UnitsError and ConcurrencyError don't really belong there, but we could import them into ndsl.exceptions.py

I don't believe AsyncResult and NullAsyncResult belong here either, to be honest.

Will remove and push again

…sult and NullAsyncResult out of typing

FlorianDeconinck

Good job.

oelbert

Great, thanks!

Adding and updating __init__ files for modules

b83f882

fmalatino requested review from FlorianDeconinck, xyuan, oelbert, bensonr and mlee03 February 14, 2024 17:44

FlorianDeconinck requested changes Feb 14, 2024

View reviewed changes

ndsl/__init__.py Show resolved Hide resolved

fmalatino marked this pull request as draft February 15, 2024 20:46

fmalatino changed the base branch from main to develop February 16, 2024 17:28

oelbert requested changes Feb 20, 2024

View reviewed changes

ndsl/comm/__init__.py Outdated Show resolved Hide resolved

ndsl/comm/__init__.py Outdated Show resolved Hide resolved

ndsl/dsl/dace/__init__.py Outdated Show resolved Hide resolved

ndsl/dsl/__init__.py Outdated Show resolved Hide resolved

ndsl/__init__.py Show resolved Hide resolved

Changes suggested from PR and updates to what is exposed

2b1cb3a

fmalatino requested review from FlorianDeconinck and oelbert February 26, 2024 20:45

FlorianDeconinck reviewed Feb 27, 2024

View reviewed changes

ndsl/dsl/dace/__init__.py Outdated Show resolved Hide resolved

tests/dsl/__init__.py Outdated Show resolved Hide resolved

Changes to missed exposed modules and clean-up of comments

7d333b5

fmalatino requested a review from FlorianDeconinck February 27, 2024 17:56

fmalatino marked this pull request as ready for review February 27, 2024 18:15

Merge branch 'develop' into feature/init_standard

ea1b4dc

fmalatino marked this pull request as draft February 27, 2024 19:05

Adding exposure for modules needed by external modules pyFV3 and pySH…

fba0d95

…iELD

fmalatino marked this pull request as ready for review February 27, 2024 20:52

oelbert requested changes Feb 27, 2024

View reviewed changes

Updated import method to mimic other package styles

8400c83

fmalatino requested a review from oelbert February 28, 2024 21:01

fmalatino and others added 3 commits March 5, 2024 10:46

Merge branch 'develop' into feature/init_standard

2018479

Exposure changes and creation of ndsl.typing module

1c8b4b9

Linting

a59736e

oelbert requested changes Mar 6, 2024

View reviewed changes

fmalatino and others added 2 commits March 6, 2024 13:12

Merge branch 'develop' into feature/init_standard

d15abe9

Changes as of comments from 6 Mar 2024, from PR 14

9f0477a

fmalatino requested a review from oelbert March 6, 2024 20:29

FlorianDeconinck requested changes Mar 7, 2024

View reviewed changes

Changes requested 1000 7 Mar 2024, PR 14 from Florian

a40a026

fmalatino requested a review from FlorianDeconinck March 7, 2024 15:46

FlorianDeconinck requested changes Mar 7, 2024

View reviewed changes

fmalatino added 2 commits March 7, 2024 14:15

Changes from comments (pokes)

13f4f1a

Poke changes

14dc28b

fmalatino requested a review from FlorianDeconinck March 8, 2024 16:26

FlorianDeconinck previously approved these changes Mar 8, 2024

View reviewed changes

oelbert approved these changes Mar 11, 2024

View reviewed changes

Imported UnitsError and ConcurrencyError to exceptions, moved AsyncRe…

f813fb1

…sult and NullAsyncResult out of typing

fmalatino dismissed FlorianDeconinck’s stale review via f813fb1 March 11, 2024 15:02

fmalatino requested review from oelbert and FlorianDeconinck March 11, 2024 15:02

FlorianDeconinck approved these changes Mar 11, 2024

View reviewed changes

oelbert approved these changes Mar 11, 2024

View reviewed changes

FlorianDeconinck merged commit 52b26a2 into develop Mar 11, 2024
3 checks passed

FlorianDeconinck deleted the feature/init_standard branch March 11, 2024 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding and updating init files for modules #14

Adding and updating init files for modules #14

fmalatino commented Feb 14, 2024 •

edited

Loading

FlorianDeconinck left a comment

FlorianDeconinck commented Feb 16, 2024

oelbert left a comment

FlorianDeconinck left a comment

oelbert left a comment

oelbert Feb 27, 2024

FlorianDeconinck Mar 6, 2024

oelbert Feb 27, 2024

FlorianDeconinck Mar 7, 2024

fmalatino Mar 7, 2024

FlorianDeconinck Mar 7, 2024

fmalatino Mar 8, 2024

oelbert left a comment

oelbert Mar 6, 2024

oelbert Mar 6, 2024

oelbert Mar 6, 2024

oelbert Mar 6, 2024

FlorianDeconinck Mar 6, 2024

FlorianDeconinck commented Mar 6, 2024

FlorianDeconinck left a comment

FlorianDeconinck Mar 7, 2024

FlorianDeconinck left a comment

FlorianDeconinck Mar 7, 2024

FlorianDeconinck Mar 7, 2024

FlorianDeconinck Mar 7, 2024

FlorianDeconinck left a comment

oelbert left a comment

oelbert Mar 8, 2024

oelbert Mar 8, 2024

fmalatino Mar 11, 2024

FlorianDeconinck left a comment

oelbert left a comment

		from .types import Allocator, AsyncRequest, NumpyModule
		from .units import UnitsError

		@@ -1,3 +1,2 @@
		from .netcdf_monitor import NetCDFMonitor
		from .protocol import Monitor

Adding and updating __init__ files for modules #14

Adding and updating __init__ files for modules #14

Conversation

fmalatino commented Feb 14, 2024 • edited Loading

FlorianDeconinck left a comment

Choose a reason for hiding this comment

FlorianDeconinck commented Feb 16, 2024

oelbert left a comment

Choose a reason for hiding this comment

FlorianDeconinck left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianDeconinck commented Mar 6, 2024

FlorianDeconinck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianDeconinck 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

FlorianDeconinck left a 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

Choose a reason for hiding this comment

FlorianDeconinck left a comment

Choose a reason for hiding this comment

oelbert left a comment

Choose a reason for hiding this comment

Adding and updating init files for modules #14

Adding and updating init files for modules #14

fmalatino commented Feb 14, 2024 •

edited

Loading