Rename BoxLib frontend as AMReX #4845

henrynjones · 2024-03-05T21:27:43Z

PR Summary

This PR renames the boxlib frontend to amrex as BoxLib is deprecated and has been superseded by AMReX. A container boxlib frontend which points to the renamed amrex frontend has been added for backwards compatibility.

PR Checklist

Adds a test for any bugs fixed. Adds tests for new features.
New features are documented, with docstrings and narrative docs

welcome · 2024-03-05T21:27:45Z

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

matthewturk · 2024-03-05T21:28:33Z

This is great!

neutrinoceros

To avoid having to maintain both path forever, I suggest deprecating the historic frontend (while of course keeping compatibility shims for now !).
This should be done using yt._maintainance.deprecation.issue_deprecation_warning

henrynjones · 2024-03-06T19:10:10Z

@neutrinoceros Sounds good!
I am thinking the deprecation warning should be issued in the Boxlib init file. How does this look?

from yt._maintenance.deprecation import issue_deprecation_warning

issue_deprecation_warning("The historic 'boxlib' frontend is deprecated as it has been renamed 'amrex'."
				"Future work should reference the 'amrex' frontend.")

Would this be the correct place for the warning? Thank you.

neutrinoceros · 2024-03-06T21:50:04Z

I'm not sure. It should be somewhere that users are expected to access, but not internal machinery, which would be a job for the api.py module. Note that we could also use a module-level __getattr__ method to personalize each warning to the exact function/class that's imported.

henrynjones · 2024-03-08T19:21:24Z

@neutrinoceros, thank you for your help. Because the underlying classes and functions haven't changed but have just moved, I have been thinking that the __getattrs__ method your suggested might not be necessary. It seems like we just want to warn people when their existing scripts reference the location of the classes/functions as within the boxlib containers/shims. Does this seem correct?

How does the following look for the desired behavior? It would be nice to issue a warning upon from yt.frontends import boxlib but I haven't found a way to do this without comprising the yt.load() output. This behavior has been accomplished by issuing the deprecation warning in the __init__ file of the fields, tests, data_structures, and io shims.

>>>import yt
>>>#load some warpx plotfile data
>>>yt.load("/home/hjones/Desktop/warpx_sims/diags/diag1000010")

yt : [INFO     ] 2024-03-08 10:27:55,280 Parameters: current_time              = 4.606746751464227e-15
yt : [INFO     ] 2024-03-08 10:27:55,281 Parameters: domain_dimensions         = [1024  512    1]
yt : [INFO     ] 2024-03-08 10:27:55,282 Parameters: domain_left_edge          = [-0.0001  0.      0.    ]
yt : [INFO     ] 2024-03-08 10:27:55,282 Parameters: domain_right_edge         = [1.e-04 1.e-04 1.e+00]
WarpXDataset: /home/hjones/Desktop/warpx_sims/diags/diag1000010

>>>from yt.frontends import boxlib
#no warning issued

>>>from yt.frontends.boxlib import api
#no warning issued

>>>from yt.frontends.boxlib.api import WarpXDataset
#no warning issued

>>>from yt.frontends.boxlib import fields
/tmp/ipykernel_27767/2643482433.py:1: DeprecationWarning: The historic 'boxlib' frontend is 
deprecated as it has been renamed 'amrex'. Future work should reference the 'amrex' frontend.
Deprecated since yt TBD

>>>from yt.frontends.boxlib import data_structures
/tmp/ipykernel_27767/2643482433.py:1: DeprecationWarning: The historic 'boxlib' frontend is 
deprecated as it has been renamed 'amrex'. Future work should reference the 'amrex' frontend.
Deprecated since yt TBD

>>>ds = data_structures.WarpXDataset("/home/hjones/Desktop/warpx_sims/diags/diag1000010")
yt : [INFO     ] 2024-03-08 10:27:55,359 Parameters: current_time              = 4.606746751464227e-15
yt : [INFO     ] 2024-03-08 10:27:55,360 Parameters: domain_dimensions         = [1024  512    1]
yt : [INFO     ] 2024-03-08 10:27:55,360 Parameters: domain_left_edge          = [-0.0001  0.      0.    ]
yt : [INFO     ] 2024-03-08 10:27:55,361 Parameters: domain_right_edge         = [1.e-04 1.e-04 1.e+00]

>>>from yt.frontends.boxlib import tests
/tmp/ipykernel_27767/3714129896.py:1: DeprecationWarning: The historic 'boxlib' frontend is 
deprecated as it has been renamed 'amrex'. Future work should reference the 'amrex' frontend.
Deprecated since yt TBD
  from yt.frontends.boxlib import tests

>>>from yt.frontends.boxlib import io
/tmp/ipykernel_27767/2721446491.py:1: DeprecationWarning: The historic 'boxlib' frontend is 
deprecated as it has been renamed 'amrex'. Future work should reference the 'amrex' frontend.
Deprecated since yt TBD
  from yt.frontends.boxlib import io

>>>ds = io.IOHandlerBoxlib("/home/hjones/Desktop/warpx_sims/diags/diag1000010")
#no warning issued

Of course since the warning is issued in __init__, we only get the warning once when we first access the shim. So with a new kernel we can get

>>> from yt.frontends.boxlib.data_structures import WarpXDataset
<stdin>:1: DeprecationWarning: The historic 'boxlib' frontend is 
deprecated as it has been renamed 'amrex'. Future work should reference the 'amrex' frontend.
Deprecated since yt TBD
>>> from yt.frontends.boxlib import data_structures
#no warning issued

I'm not sure if a single warning is desired. As can be seen I left the api.py module alone. I am happy to work on the __getattr__ approach if you think that would best. Thanks!

Note I have just pushed these updates mostly to try the full test suite build again as I failed it most recently.

neutrinoceros

The warnings seem fine (although to be nitpicky I should point out that not just "future work" should be advised to switch to drop deprecated APIs).
I would like that we try to refactor this to avoid repeating code though. Otherwise we might miss some of the repetitions when/if we need to edit the warning.

yt/frontends/__init__.py

neutrinoceros · 2024-03-09T07:51:00Z

Not using __getattr__ is fine, btw.

Regarding CI, you'll need to edit

pyproject.toml (specifically [tool.pytest.ini_options])
tests/tests.yaml

greping for "boxlib" in these files should give you an idea what is needed there.
That said I'm not sure how best to migrate the answer tests registry (tests/tests.yaml). @Xarthisius might know what to do here.

henrynjones · 2024-03-11T20:54:14Z

The warnings seem fine (although to be nitpicky I should point out that not just "future work" should be advised to switch to drop deprecated APIs). I would like that we try to refactor this to avoid repeating code though. Otherwise we might miss some of the repetitions when/if we need to edit the warning.

That makes sense, thanks for the help! I have added a deprecation.py file to the boxlib frontend shim containing

#deprecation.py
from yt._maintenance.deprecation import issue_deprecation_warning, warnings

def boxlib_deprecation():
    with warnings.catch_warnings():
        warnings.simplefilter('default')
        issue_deprecation_warning(
            "The historic 'boxlib' frontend is \n"
            "deprecated as it has been renamed 'amrex'. "
            "Existing and future work should instead reference the 'amrex' frontend.",
            stacklevel=3,
            since="TBD",
        )

Then in the __init__ of io, fields, tests, and data_structures we call

from ..deprecation import boxlib_deprecation 
boxlib_deprecation()

neutrinoceros · 2024-03-11T21:20:03Z

Let's make this module explicitly private while we can then deprecation.py -> _deprecation.py

yt/frontends/boxlib/_deprecation.py

henrynjones · 2024-03-12T01:07:40Z

Not using __getattr__ is fine, btw.

Regarding CI, you'll need to edit

pyproject.toml (specifically [tool.pytest.ini_options])

tests/tests.yaml

greping for "boxlib" in these files should give you an idea what is needed there. That said I'm not sure how best to migrate the answer tests registry (tests/tests.yaml). @Xarthisius might know what to do here.

As for the CI tasks, I have added amrex sections to the pyproject.toml and tests/tests.yaml. For now I have just copied this over but I'm sure there are better ideas for migrating the answer tests registry as you suggest.

atmyers · 2024-03-13T20:58:06Z

tests/tests.yaml

@@ -3,6 +3,28 @@ answer_tests:
  local_art_004: # PR 3081, 3101
    - yt/frontends/art/tests/test_outputs.py:test_d9p

+#copied from boxlib frontend
+  local_amrex_012:


does it make sense, for the purposes of this PR, to make sure the answer tests pass with this named 'local_boxlib_012, and same for below, and then change the name to local_amrex_012 in a follow-on PR? What do you think @Xarthisius?

I think this is the last point we're waiting for on this PR, so let me ping @Xarthisius again.

atmyers · 2024-03-14T23:35:57Z

Actually for the boxlib answer tests, does it make sense to still have a test_outputs.py in there that runs the same tests, but imports everything from yt.frontends.boxlib.api, since we still want that to work?

neutrinoceros · 2024-03-15T07:37:05Z

IIRC These tests are quite expensive to run, so I don't think we should double them. However, adding a minimal test to check that the backward compatibility layer still works (and properly raises the warning) would indeed be useful.

neutrinoceros · 2024-04-01T13:09:45Z

I just ran a small check locally and discovered that a loading a AMRex dataset triggers a ResourceWarning

/Users/clm/dev/yt-project/yt/yt/frontends/boxlib/data_structures/__init__.py:18: DeprecationWarning: The historic 'boxlib' frontend is 
deprecated as it has been renamed 'amrex'. Existing and future work should instead reference the 'amrex' frontend.
Deprecated since yt TBD
  boxlib_deprecation()
/Users/clm/dev/yt-project/yt/yt/frontends/amrex/data_structures.py:701: ResourceWarning: unclosed file <_io.TextIOWrapper name='/Users/clm/dev/yt-project/yt/amrex_test/plt.Cavity00010/inputs' mode='r' encoding='UTF-8'>
  lines = [line.lower() for line in open(cparam_filepath).readlines()]
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/Users/clm/dev/yt-project/yt/yt/frontends/amrex/data_structures.py:728: ResourceWarning: unclosed file <_io.TextIOWrapper name='/Users/clm/dev/yt-project/yt/amrex_test/plt.Cavity00010/Header' mode='r' encoding='UTF-8'>
  self._parse_header_file()
ResourceWarning: Enable tracemalloc to get the object allocation traceback

specifically I used the ParticleCavity dataset from https://yt-project.org/data/
and loaded with the following script:

import yt
from yt.frontends.boxlib.data_structures import AMReXDataset

ds = yt.load("plt.Cavity00010")

this needs to be fixed before we can merge this PR.

yut23

It looks like the ResourceWarnings are due to the warnings filter not being restored properly.

yut23 · 2024-05-01T01:18:01Z

yt/frontends/boxlib/_deprecation.py

+    warnings.simplefilter("always")
+    issue_deprecation_warning(
+        "The historic 'boxlib' frontend is \n"
+        "deprecated as it has been renamed 'amrex'. "
+        "Existing and future work should instead reference the 'amrex' frontend.",
+        stacklevel=3,
+        since="TBD",
+    )
+    warnings.resetwarnings()


This block needs to be put inside a with warnings.catch_warnings():, so as to not clobber the user's warning settings (leading to the extra ResourceWarnings @neutrinoceros noticed).

yut23 · 2024-05-01T01:21:26Z

yt/frontends/boxlib/tests/test_boxlib_deprecation.py

+                issubclass(w[2].category, DeprecationWarning),
+            ]
+        )
+    warnings.resetwarnings()


This resetwarnings() call should be deleted, as it may affect other tests. catch_warnings() will restore the original warnings filter when it exits.

Ok made these changes. Thanks!

This one still needs updating

Sorry looks like I didn't get here, just needs the warnings.resewarnings() deleted.

neutrinoceros · 2024-05-01T06:11:59Z

this needs to be fixed before we can merge this PR.

Thanks to @yut23, I understand now that the problem exists on main too, so I want to clarify that it's not a blocker to this PR after all. However Eric's suggestions would still avoid making the situation worse.

henrynjones · 2024-05-01T17:28:38Z

this needs to be fixed before we can merge this PR.

Thanks to @yut23, I understand now that the problem exists on main too, so I want to clarify that it's not a blocker to this PR after all. However Eric's suggestions would still avoid making the situation worse.

Ok great, thank you @yut23 for investigating. How should I go about reconciling this branch and the new PR #4891 ? I am happy to copy those changes over to the amrex frontend but I don't want to add confusion.

henrynjones · 2024-05-01T19:12:35Z

this needs to be fixed before we can merge this PR.

Thanks to @yut23, I understand now that the problem exists on main too, so I want to clarify that it's not a blocker to this PR after all. However Eric's suggestions would still avoid making the situation worse.

Ok great, thank you @yut23 for investigating. How should I go about reconciling this branch and the new PR #4891 ? I am happy to copy those changes over to the amrex frontend but I don't want to add confusion.

On second thought I will wait until the bug fix PR is merged and then merge those changes into the amrex frontend.

neutrinoceros · 2024-05-01T21:11:41Z

Or we could go with this one first and then rebase @yut23's, which I expect would be less risky and easier to follow, if that's okay with you both.

yut23 · 2024-05-01T23:04:57Z

I'm fine with rebasing mine.

henrynjones · 2024-05-02T23:15:26Z

I made the requested changes to the _deprecation file so I think deciding on the tests are the last thing. Thanks!

yut23 · 2024-05-06T21:09:09Z

yt/frontends/boxlib/_deprecation.py

+            "The historic 'boxlib' frontend is \n"
+            "deprecated as it has been renamed 'amrex'. "
+            "Existing and future work should instead reference the 'amrex' frontend.",
+            stacklevel=3,


Should point to the import line rather than boxlib_deprecation()

Suggested change

stacklevel=3,

stacklevel=4,

yut23 · 2024-05-06T21:15:37Z

Looks like the branch got mixed up with #4848. Also, I don't think the yt.frontends.boxlib.test module should raise a deprecation warning: when I was running pytest locally, it raised the warning during test discovery as well as execution.

henrynjones · 2024-05-09T21:25:50Z

Looks like the branch got mixed up with #4848. Also, I don't think the yt.frontends.boxlib.test module should raise a deprecation warning: when I was running pytest locally, it raised the warning during test discovery as well as execution.

Thanks, sorry I missed that. Now I get no differences with git diff ytmain/main yt/frontends/open_pmd.

neutrinoceros · 2024-05-11T13:27:20Z

can you please rebase again to fix a new conflict ? I think we should be able to merge after that, sorry for the inconvenience and thank you for your patience !

henrynjones · 2024-05-15T16:27:36Z

Ok I just rebased and purged out dupe commits as requested. I'm not sure exactly what happened during your attempt I suspect you forgot (or didn't know) to resolve merge conflicts.

I also made a copy of the branch as I found it and pushed it to my fork under save_4537, just in case I wasn't careful enough. Anyway it should be okay now. I can sign it off now but I'll give others another opportunity to approve or request changes. If no one objects to it we can merge this in a week or so. Thanks again for your work and patience !

Thanks for your help!

henrynjones · 2024-05-15T16:36:44Z

pyproject.toml

@@ -116,7 +117,7 @@ http-stream = ["requests>=2.20.0"]
 idefix = ["yt_idefix[HDF5]>=2.3.0"] # externally packaged frontend
 moab = ["yt[HDF5]"]
 nc4-cm1 = ["yt[netCDF4]"]
-open-pmd = ["yt[HDF5]"]
+open-pmd = ["yt[openpmd_api]"]


This snuck in from my other PR and should be back to HDF5.

henrynjones · 2024-05-15T16:42:08Z

yt/frontends/boxlib/tests/test_boxlib_deprecation.py

+                issubclass(w[2].category, DeprecationWarning),
+            ]
+        )
+    warnings.resetwarnings()


Sorry looks like I didn't get here, just needs the warnings.resewarnings() deleted.

neutrinoceros · 2024-05-15T16:50:11Z

You can apply these changes yourself. If you're hesitant about how to get back in sync after my rebase, here's how you can do it

git checkout main
git branch -D boxlib_to_amrex
git fetch origin
git checkout boxlib_to_amrex

henrynjones · 2024-05-15T19:12:52Z

You can apply these changes yourself. If you're hesitant about how to get back in sync after my rebase, here's how you can do it
git checkout main
git branch -D boxlib_to_amrex
git fetch origin
git checkout boxlib_to_amrex

Ok I have fixed my two issues and done this process. I am not sure why your rebase looked like only one commit while mine shows every commit, but clearly I have a lot of git learning to do. I wish the log looked nicer but I think its good?

zingale · 2024-05-15T19:16:10Z

You can just squash on merge for the PR and it will look clean.

yut23

Looks good to me! 😄

neutrinoceros requested changes Mar 6, 2024

View reviewed changes

henrynjones marked this pull request as ready for review March 8, 2024 20:48

neutrinoceros reviewed Mar 9, 2024

View reviewed changes

yt/frontends/__init__.py Show resolved Hide resolved

neutrinoceros reviewed Mar 11, 2024

View reviewed changes

yt/frontends/boxlib/_deprecation.py Outdated Show resolved Hide resolved

atmyers reviewed Mar 13, 2024

View reviewed changes

yut23 mentioned this pull request Apr 30, 2024

BUG: ResourceWarnings for unclosed files in boxlib frontend #4890

Open

yut23 requested changes May 1, 2024

View reviewed changes

neutrinoceros added this to the 4.4.0 milestone May 2, 2024

yut23 reviewed May 6, 2024

View reviewed changes

neutrinoceros added enhancement Making something better deprecation deprecate features or remove deprecated ones refactor improve readability, maintainability, modularity and removed enhancement Making something better labels May 15, 2024

henrynjones commented May 15, 2024

View reviewed changes

Henry Jones and others added 13 commits May 15, 2024 12:02

Renamed boxlib frontend to amrex

06a6e2d

Updated inits

4ddb8d9

Fixed test_outputs import

0abc89a

Issue deprecation warnings

4a42088

Refactor and CI tasks

5b8b1b4

Made deprecation module private

07c3988

Set deprecation warning filter to always

d2cb885

Added import test, removed warning catch in _deprecation

7e7e466

Make 'boxlib' tests minimal

c5dee4b

Make test_imports() reload modules

a63ea77

Add catch_warnings()

6de10d5

Removed tests deprecation and pointed deprecation to import

f91fffb

Updated pyproject dependency

537c578

henrynjones force-pushed the boxlib_to_amrex branch from 92b6c59 to 537c578 Compare May 15, 2024 19:05

Fixed pyproject dependecy and removed warnings_reset

9da50db

henrynjones dismissed stale reviews from neutrinoceros and zingale via 9da50db May 15, 2024 19:10

yut23 approved these changes May 16, 2024

View reviewed changes

zingale approved these changes May 16, 2024

View reviewed changes

atmyers approved these changes May 16, 2024

View reviewed changes

neutrinoceros mentioned this pull request May 18, 2024

escape molecules for boxlib datasets #4908

Open

2 tasks

Rename BoxLib frontend as AMReX #4845

Are you sure you want to change the base?

Rename BoxLib frontend as AMReX #4845

Conversation

henrynjones commented Mar 5, 2024 • edited

PR Summary

PR Checklist

welcome bot commented Mar 5, 2024

matthewturk commented Mar 5, 2024

neutrinoceros left a comment

Choose a reason for hiding this comment

henrynjones commented Mar 6, 2024 • edited

neutrinoceros commented Mar 6, 2024

henrynjones commented Mar 8, 2024 • edited

neutrinoceros left a comment

Choose a reason for hiding this comment

neutrinoceros commented Mar 9, 2024

henrynjones commented Mar 11, 2024

neutrinoceros commented Mar 11, 2024

henrynjones commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atmyers commented Mar 14, 2024

neutrinoceros commented Mar 15, 2024

neutrinoceros commented Apr 1, 2024

yut23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yut23 May 1, 2024 • edited

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

neutrinoceros commented May 1, 2024

henrynjones commented May 1, 2024

henrynjones commented May 1, 2024

neutrinoceros commented May 1, 2024

yut23 commented May 1, 2024

henrynjones commented May 2, 2024

Choose a reason for hiding this comment

yut23 commented May 6, 2024

henrynjones commented May 9, 2024 • edited

neutrinoceros commented May 11, 2024

henrynjones commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutrinoceros commented May 15, 2024

henrynjones commented May 15, 2024

zingale commented May 15, 2024

yut23 left a comment

Choose a reason for hiding this comment

henrynjones commented Mar 5, 2024 •

edited

henrynjones commented Mar 6, 2024 •

edited

henrynjones commented Mar 8, 2024 •

edited

yut23 May 1, 2024 •

edited

henrynjones commented May 9, 2024 •

edited