Bump boutdata, boututils versions #2335

ZedThree · 2021-05-28T14:28:37Z

Should also go into master/v4.4?

dschwoerer · 2021-05-28T14:43:49Z

Should we prefer the pip version?
The git versions spit out warnings if some optional dependencies are not installed ...

ZedThree · 2021-05-28T15:05:16Z

I thought we had some fix so that setuptools_scm didn't spit out warnings if we're using the bundled submodules?

Multiple test failures, all of this form:

Traceback (most recent call last):
  File "./runtest", line 101, in <module>
    restart.redistribute(nproc, path=restartdir, output="data")
  File "/home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boutdata/restart.py", line 658, in redistribute
    v, xguards=True, yguards=True, info=False, datafile_cache=DataFileCache
  File "/home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boutdata/collect.py", line 230, in collect
    nfiles=len(file_list),
  File "/home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boutdata/collect.py", line 1015, in _get_grid_info
    ny_inner = int(load_and_check("ny_inner"))
  File "/home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boutdata/collect.py", line 989, in load_and_check
    raise ValueError("Missing {} variable".format(varname))
ValueError: Missing ny_inner variable

johnomotani · 2021-05-29T13:19:20Z

I thought we had some fix so that setuptools_scm didn't spit out warnings if we're using the bundled submodules?

We added a work-around so that if setuptools_scm isn't installed there's no error and boutdata.__version__ is set to "dev", but a warning is emitted when doing so. My favourite answer would be for the warning to only be emitted when boutdata.__version__ is actually read/used, but as far as I can see from a quick google, it has to be a str evaluated when __init__.py is executed, so I don't see how this would be possible. One option would be to use warnings.filterwarnings() (https://docs.python.org/3/library/warnings.html#warnings.filterwarnings) to ignore this specific warning when running the test suite.

Should we prefer the pip version?

Is that an easy thing to do? Git submodules are very convenient, but I'd assumed required using the git-repo version?

Multiple test failures, all of this form:

Test failures (at least test-laplace that I looked at) are because the benchmark data is missing an ny_inner (and jyseps*, ixseps*) - I guess because the data was generated with a very old version of BOUT++, or generated by hand and these variables weren't strictly necessary. Previously this was not a problem because they were not read unless yguards=True was set. Possible solutions:

return to the previous behaviour. In _get_grid_info() we could use the value of yguards to skip some variables when they're not needed. Downside is that the dict returned by _get_grid_info() contains different variables depending on the arguments to the function, which could be confusing.
set some default values for ny_inner, etc. if they are missing. Downside is that this might mask actual errors or corrupt data files, or I guess somehow produce incorrect results.
update the benchmark.nc, etc., files so that they do contain ny_inner, etc. Downside is adding new copies of binary files to the repo.

I think I'd vote for 1. What does everyone else think?

johnomotani · 2021-05-30T16:04:11Z

boutproject/boutdata#46 is a fix for the collect errors in the tests here (implementing 'option 1').

johnomotani

Looks good to me (as long as the tests pass). I think it would be good to bump the versions in master/4.4 as well.

ZedThree · 2021-06-01T14:07:22Z

We're running pytest here which is now picking up the boutdata/boututils tests and failing because something's going wrong with the fixtures. This is creating a lot of output in the action logs.

We should either drop pytest now, or be more specific about what we want to test (just Zoidberg I think? and even that's going to be moved out)

The CMake build is also failing on test-squash:

Traceback (most recent call last):
  File "/home/runner/work/BOUT-dev/BOUT-dev/build/tests/integrated/test-squash/runtest", line 94, in <module>
    timed_shell_safe("{} -qdcal 9 data --outputname ../f2.nc".format(bout_squashoutput))
  File "/home/runner/work/BOUT-dev/BOUT-dev/build/tests/integrated/test-squash/runtest", line 38, in timed_shell_safe
    shell_safe(cmd, *args, **kwargs)
  File "/home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boututils/run_wrapper.py", line 269, in shell_safe
    raise RuntimeError(
RuntimeError: Run failed with 1.
Command was:
/home/runner/work/BOUT-dev/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput -qdcal 9 data --outputname ../f2.nc

Output was

None

which is not terribly helpful

dschwoerer · 2021-06-01T14:21:38Z

The output is not captured, and is thus printed above:

2021-06-01T13:52:25.5564051Z Writing options to file data/BOUT.settings
2021-06-01T13:52:25.5564691Z 	Option time_report:show = 0 (default)
2021-06-01T13:52:25.5566724Z /home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boututils/__init__.py:28: UserWarning: 'setuptools_scm' and git are required to get the version number when running boututils from the git repo. Please install 'setuptools_scm' and check 'git rev-parse HEAD' works. Setting __version__='dev' as a workaround.
2021-06-01T13:52:25.5568194Z   warn(
2021-06-01T13:52:25.5570083Z /home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boutdata/__init__.py:31: UserWarning: 'setuptools_scm' and git are required to get the version number when running boutdata from the git repo. Please install 'setuptools_scm' and check 'git rev-parse HEAD' works. Setting __version__='dev' as a workaround.
2021-06-01T13:52:25.5571637Z   warn(
2021-06-01T13:52:25.5572098Z Traceback (most recent call last):
2021-06-01T13:52:25.5573336Z   File "/home/runner/work/BOUT-dev/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput", line 56, in <module>
2021-06-01T13:52:25.5574360Z     squash.squashoutput(**args.__dict__)
2021-06-01T13:52:25.5575549Z   File "/home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boutdata/squashoutput.py", line 184, in squashoutput
2021-06-01T13:52:25.5576518Z     dims = outputs.dimensions[varname]
2021-06-01T13:52:25.5577532Z AttributeError: 'BoutOutputs' object has no attribute 'dimensions'
2021-06-01T13:52:25.5579719Z /home/runner/work/BOUT-dev/BOUT-dev/tools/pylib/boututils/__init__.py:28: UserWarning: 'setuptools_scm' and git are required to get the version number when running boututils from the git repo. Please install 'setuptools_scm' and check 'git rev-parse HEAD' works. Setting __version__='dev' as a workaround.

I think the correct fix would be to drop bout-squashoutput and use boutdata's one.

johnomotani · 2021-06-01T14:35:03Z

I think the correct fix would be to drop bout-squashoutput and use boutdata's one.

👍 If we make the script in boutdata executable (boutproject/boutdata#47), we should be able to replace bin/bout-squashoutput with a symbolic link ln -s ../externalpackages/boutdata/boutdata/scripts/bout_squashoutput.py bout-squashoutput.

johnomotani · 2021-06-01T14:38:15Z

We're running pytest here which is now picking up the boutdata/boututils tests and failing because something's going wrong with the fixtures. This is creating a lot of output in the action logs.

I don't think we want to run the boutdata/boututils tests here. They are fairly long now, and have already been checked on the separate repos.

I'd guess the error with the tmp_path fixture is due to not having a new enough version of pytest.

ZedThree · 2021-06-11T15:20:37Z

I'm still having issues with test-squash locally:

  File "/home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput", line 97, in <module>
    main()
  File "/home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput", line 93, in main
    squash.squashoutput(**args.__dict__)
  File "/home/peter/Codes/BOUT-dev/tools/pylib/boutdata/squashoutput.py", line 184, in squashoutput
    dims = outputs.dimensions[varname]
AttributeError: 'BoutOutputs' object has no attribute 'dimensions'
/home/peter/Codes/BOUT-dev/tools/pylib/boututils/__init__.py:28: UserWarning: 'setuptools_scm' and git are required to get the version number when running boututils from the git repo. Please install 'setuptools_scm' and check 'git rev-parse HEAD' works. Setting __version__='dev' as a workaround.
  warn(
Making Squash test
  2.15512705s ./squash -q -q -q nout=2
  0.00451946s mv data/BOUT.dmp.0.nc f1.nc
  0.00583148s rm -f f2.nc
  2.37416577s ./squash -q -q -q nout=2
  1.27090645s /home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput -qdcl 9 data --outputname ../f2.nc
  0.17930675s verify f1.nc f2.nc
  0.00783873s rm -f f2.nc
  2.33731103s ./squash -q -q -q
  1.26911783s /home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput -qdcl 9 data --outputname ../f2.nc
  2.32363844s ./squash -q -q -q restart
  0.45733857s /home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput -qdcal 9 data --outputname ../f2.nc
Traceback (most recent call last):
  File "/home/peter/Codes/BOUT-dev/build/tests/integrated/test-squash/runtest", line 94, in <module>
    timed_shell_safe("{} -qdcal 9 data --outputname ../f2.nc".format(bout_squashoutput))
  File "/home/peter/Codes/BOUT-dev/build/tests/integrated/test-squash/runtest", line 38, in timed_shell_safe
    shell_safe(cmd, *args, **kwargs)
  File "/home/peter/Codes/BOUT-dev/tools/pylib/boututils/run_wrapper.py", line 269, in shell_safe
    raise RuntimeError(
RuntimeError: Run failed with 1.
Command was:
/home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput -qdcal 9 data --outputname ../f2.nc

Output was

None

And then trying to run the bout-squashoutput line manually I get:

  File "/home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput", line 97, in <module>
    main()
  File "/home/peter/Codes/BOUT-dev/tests/integrated/test-squash/../../../bin/bout-squashoutput", line 93, in main
    squash.squashoutput(**args.__dict__)
  File "/home/peter/Codes/BOUT-dev/tools/pylib/boutdata/squashoutput.py", line 132, in squashoutput
    outputs = BoutOutputs(
  File "/home/peter/Codes/BOUT-dev/tools/pylib/boutdata/data.py", line 1031, in __init__
    latest_file = max(temp_file_list, key=os.path.getctime)
ValueError: max() arg is an empty sequence
Exception ignored in: <function BoutOutputs.__del__ at 0x7fed83ed5dc0>
Traceback (most recent call last):
  File "/home/peter/Codes/BOUT-dev/tools/pylib/boutdata/data.py", line 1141, in __del__
    if self._parallel is not False:
AttributeError: 'BoutOutputs' object has no attribute '_parallel'

johnomotani · 2021-06-11T16:12:49Z

I think I've found the cause of the error - when squashoutput 'appends' it uses BoutOutputs.dimensions, which probably existed at some point but got taken out when refactoring. Adding in a getter-type property fixes the test (I'll push a fix to the boutdata repo). This shows that better test coverage of squashoutput options in the boutdata tests would be useful, if anyone has the time to add more. Also the error message is awful - I'll open an issue to discuss.

johnomotani · 2021-06-11T16:24:45Z

@ZedThree boutproject/boutdata#49 should fix the test failure.

johnomotani · 2021-06-11T16:29:52Z

PS, I had trouble reproducing the error outside of the runtest script - I suspect the call that failed depended on files that were created in previous steps. I ended up tracking down the error when I put a pdb.set_trace() in squashoutput() and happened to call runtest again.

ZedThree · 2021-06-11T16:58:01Z

Just checked that branch, looks like it works fine, thanks @johnomotani !

Yes, I had exactly the same problem with needing to rerun the BOUT++ model to regenerate the outputs.

I guess the tests in boutdata should probably look like the test here, except just creating the expected files instead of needing to run a model? Not sure I'll have time to sort that out myself though.

johnomotani · 2021-06-11T17:02:15Z

I guess the tests in boutdata should probably look like the test here, except just creating the expected files instead of needing to run a model? Not sure I'll have time to sort that out myself though.

Yes, pretty much. The boutdata test suite already has functions for creating expected data - probably just need to modify one of those to split the expected data in time into two sets to be combined.

dschwoerer · 2021-06-11T17:48:24Z

.travis_script.sh

@@ -125,7 +125,6 @@ fi
 if [[ ${INTEGRATED} == 1 ]]
 then
    time make check-integrated-tests
-    time py.test-3 tools/pylib/


Maybe keep this for zoidberg?
Or rebase #2214 to coord3d_merged2?

I've dropped this because I'm assuming #2214 will go in at some point -- I'm not sure how best to handle the interaction with the 3D metrics branch.

One way might be to merge #2214 now, make sure all the 3D metric stuff is in the zoidberg repo, and then merge next into 3D metrics?

ZedThree added 2 commits May 28, 2021 15:25

Bump boutdata version to v0.1.4

16b7d4e

Bump boututils version to v0.1.7

da0c278

ZedThree added the backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 label May 28, 2021

ZedThree added this to the BOUT-5.0 milestone May 28, 2021

ZedThree requested a review from johnomotani May 28, 2021 14:28

johnomotani mentioned this pull request May 30, 2021

When reading info from grid, only read if actually needed boutproject/boutdata#46

Merged

Bump boutdata to latest version

18e14bf

johnomotani previously approved these changes Jun 1, 2021

View reviewed changes

ZedThree added 3 commits June 11, 2021 16:03

Bump boutdata to latest version

3a0ee32

Don't run pytest on GHA: all tested modules are now separate repos

7ae0285

Make bout-squashoutput a symlink to boutdata version

43f8951

ZedThree dismissed johnomotani’s stale review via 43f8951 June 11, 2021 15:14

johnomotani mentioned this pull request Jun 11, 2021

Unhelpful error messages from BoutOutputs boutproject/boutdata#48

Open

Bump boutdata to latest version

c17f5dd

dschwoerer reviewed Jun 11, 2021

View reviewed changes

ZedThree requested a review from johnomotani June 15, 2021 09:20

johnomotani approved these changes Jun 15, 2021

View reviewed changes

ZedThree merged commit c85336e into next Jun 15, 2021

ZedThree deleted the bump-boutdata-boututils branch June 15, 2021 10:09

ZedThree mentioned this pull request Aug 4, 2021

Release V4.4.0 #2383

Merged

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump boutdata, boututils versions #2335

Bump boutdata, boututils versions #2335

ZedThree commented May 28, 2021

dschwoerer commented May 28, 2021

ZedThree commented May 28, 2021

johnomotani commented May 29, 2021

johnomotani commented May 30, 2021 •

edited

johnomotani left a comment

ZedThree commented Jun 1, 2021

dschwoerer commented Jun 1, 2021

johnomotani commented Jun 1, 2021

johnomotani commented Jun 1, 2021

ZedThree commented Jun 11, 2021

johnomotani commented Jun 11, 2021

johnomotani commented Jun 11, 2021

johnomotani commented Jun 11, 2021

ZedThree commented Jun 11, 2021

johnomotani commented Jun 11, 2021

dschwoerer Jun 11, 2021

ZedThree Jun 14, 2021

Bump boutdata, boututils versions #2335

Bump boutdata, boututils versions #2335

Conversation

ZedThree commented May 28, 2021

dschwoerer commented May 28, 2021

ZedThree commented May 28, 2021

johnomotani commented May 29, 2021

johnomotani commented May 30, 2021 • edited

johnomotani left a comment

Choose a reason for hiding this comment

ZedThree commented Jun 1, 2021

dschwoerer commented Jun 1, 2021

johnomotani commented Jun 1, 2021

johnomotani commented Jun 1, 2021

ZedThree commented Jun 11, 2021

johnomotani commented Jun 11, 2021

johnomotani commented Jun 11, 2021

johnomotani commented Jun 11, 2021

ZedThree commented Jun 11, 2021

johnomotani commented Jun 11, 2021

dschwoerer Jun 11, 2021

Choose a reason for hiding this comment

ZedThree Jun 14, 2021

Choose a reason for hiding this comment

johnomotani commented May 30, 2021 •

edited