Make setup rundir more robust

[ashjbarnes]

The setup_run_directory method has two problems:

1. It assumes that the package has been installed via conda when it looks for the premade_run_directories path.
2. If reading in an existing mask table and the user hasn't run the FRE tools method, it crashes because the cpu layout hasn't been defined anywhere in this case.

These issues are addressed via some simple if-statement error handing.

[navidcy]

I'd feel more comfortable if @angus-g had a look here.

[ashjbarnes]

To be more verbose:

If you install from conda, then importlib.resources.files("regional_mom6") / "demos/premade_run_directories" lands you in the right place, namely:
pathtocode/regional_mom6/demos/premade_run_directories
However, if you import it directly then you end up one folder too deep, I.E
pathtocode/regional_mom6/regional_mom6/demos/premade_run_directories

Hence with my fix I just go back one directory if we detect that the path doesn't exist.

For the second point, I've made a default placeholder for the layout attribute so that we don't end up calling self.attribute when it doesn't exist. There are probably more elegant ways of doing this but this works.

[angus-g]

Honestly I think the better approach here would be to move demos under regional_mom6, which would also align with the PyPA recommendation (https://setuptools.pypa.io/en/latest/userguide/datafiles.html#non-package-data-files):

Instead, the PyPA recommends that any data files you wish to be accessible at run time be included inside the package.

That just requires an additional modification to the pyproject.toml:

diff --git a/pyproject.toml b/pyproject.toml
index e427db4..beee91a 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -26,9 +26,6 @@ build-backend = "setuptools.build_meta"
 [tool.setuptools]
 packages = ["regional_mom6", "regional_mom6.demos.premade_run_directories"]
 
-[tool.setuptools.package-dir]
-"regional_mom6.demos" = "demos"
-
 [tool.setuptools.package-data]
 "regional_mom6.demos.premade_run_directories" = ["**/*"]
 

[navidcy]

Let's do that then? New PR -- this one?

[ashjbarnes]

Ok cool good to know! I need to learn more about these conventions rather than making it up all the time.

so this way it will import correctly whether installed via conda or imported directly from a folder (as might be the case for testing)

[angus-g]

so this way it will import correctly whether installed via conda or imported directly from a folder (as might be the case for testing)

That's right, although it's also better for testing to do an editable install with pip install -e rather than importing the directory itself.

[ashjbarnes]

so this way it will import correctly whether installed via conda or imported directly from a folder (as might be the case for testing)

That's right, although it's also better for testing to do an editable install with pip install -e rather than importing the directory itself.

even with Conda? That's the preferred install now right?

[angus-g]

even with Conda? That's the preferred install now right?

For actually using the package, yes, but we're talking about the development case. I would suggest the best way for development is to create a conda environment for regional MOM6 (or use an existing one with all the right dependencies in place), and then use an editable install of your working copy of the repository into that environment. Then any changes within the repository are immediately available without needing any additional install, or modifications to path.

[navidcy]

How should we move forward for this?

[ashjbarnes]

For now I'd be happy with merging this PR as is (since it does address the two issues I outline in the description, however clunkily), and making an issue pointing out that a cleaner implementation would be changing the folder location and doing whatever needs to be done on the Conda side. It's a bit of an edge case issue so I'm not going to prioritise implementing a 'clean' solution right now

[navidcy]

@angus-g?

[angus-g]

I would remove the special-casing code (i.e. the added lines 1429-1447) and perhaps make a documentation suggestion for development to use editable installs.

[ashjbarnes]

I've got a headache because I tried to change the directory structure to match @angus-g 's suggestion. However, testing failed before it even needed the premade run directories! I force reset to two different commits that previously passed the tests, and they now throw this new error. Is it possible that our tests / code aren't frozen to specific versions, and so will change over time?

The current commit passed pytest 3 weeks ago. I guess the evidence of that is now hidden by my overwriting of the git history by forcing the head back in time...

[angus-g]

The answers are the same, but retriggering the test has pulled in numpy 2.0.0, which has updated in the meantime. The only difference is that it's being more strict about the expected types in the test. I can quickly fix that and you can rebase on there.

[ashjbarnes]

I also wasn't sure how to update the .toml file when moving premade_run_dirs from ~/demos_/ to ~/regional_mom6/. For instance, do we still need to explicitly mention the path to these files as package-data in

packages = ["regional_mom6", "regional_mom6.demos.premade_run_directories"]

[tool.setuptools.package-dir]
"regional_mom6.demos" = "demos"

[tool.setuptools.package-data]
"regional_mom6.demos.premade_run_directories" = ["**/*"]

[navidcy]

a test broke?

[angus-g]

Yes, we still need it as package data. We essentially just stop "remapping" where the demos directory is. The only modification you need is here: #177 (comment)

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[navidcy]

what is this np.float64 thing?

[navidcy]

it's elsewhere also....

[angus-g]

That was required as part of the numpy 2.0.0 compatibility to make the doctest pass: it prints out the types of return values. I think that is useful, but given we're not going to use numpy 2.0.0 for a little bit, it'll disappear for now anyway!

[ashjbarnes]

After merging from main, we still have what looks like a numpy 2.0 issue stopping the test. This is really confusing me! Looking at the files changed tab, this PR only really messes around the setup_rundir function. If it's been merged with main I'm not sure why we'd have numpy 2 issues

[angus-g]

It's because the docstring changes that I was experimenting with in #178 got merged in here. In the end, we needed to pin numpy < 2.0.0, so the changes to docstrings can be reverted.

[ashjbarnes]

I've made a new PR #179 that does the same of this one but with a clean git history! Also the checks pass since it's now branched right off main, which @angus-g fixed up regarding the Numpy 2 bug

[navidcy]

@ashjbarnes can you open an issue linking to the discussion in this PR regarding the improved method that @angus-g suggested so we don't forget about it?