[SCons] Check for minimum version of ruamel.yaml #891

bryanwweber · 2020-07-02T19:29:28Z

To run the tests of the converter scripts, ruamel.yaml must be installed
with a minumum version at least 0.15.94. We aren't sure what the true
minimum version is, but 0.15.94 was released in April 2019 so is
probably widely released enough to count. Note that the version
available from the Ubuntu 16.04 repositories is not new

If applicable, fill in the issue number this pull request is fixing

Fixes #831

Checklist

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
The pull request is ready for review

ischoegl · 2020-07-02T20:17:25Z

test/SConscript

@@ -268,6 +268,33 @@ test_root = '#interfaces/cython/cantera/test'
 for f in mglob(localenv, test_root, '^test_*.py'):
    python_subtests.append(f.name[5:-3])

+if localenv['python_package'] != 'none':
+    # We choose ruamel.yaml 0.15.94 as the minimum version,


0.15.87 ... ?

Actually

ruamel_yaml-0.15.80 |py37h8f50634_1001 267 KB conda-forge

on several of the runners.

Update: Ubuntu 18.04 uses 0.15.34 see link, and I recall that it worked out of the box. Same minimum version will work for debian. CentOS7 is older (0.13.14, although CentOS8 has 0.15.41) and like Xenial (0.10.23) throws the error.

Ok, thank you for helping triangulate this!

codecov · 2020-07-02T20:20:46Z

Codecov Report

Merging #891 into main will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #891   +/-   ##
=======================================
  Coverage   71.18%   71.18%           
=======================================
  Files         376      376           
  Lines       46201    46201           
=======================================
  Hits        32888    32888           
  Misses      13313    13313

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf1c081...16c8fe6. Read the comment docs.

speth

I'm wondering if it would make more sense to do this check in SConstruct, at the same time we do the checks for Numpy and Cython. Otherwise, we create the scenario where the user just runs scons build && scons install, never triggering this check and ending up with a broken Cantera installation.

bryanwweber · 2020-07-03T17:04:49Z

The reservation I have about putting this check in SConstruct instead of test/SConscript is that ruamel.yaml is not a build requirement, it's a runtime requirement. In the case you suggest, the user would only hit an error message when they tried to convert a mechanism, and the fix would be that ruamel.yaml isn't installed (or the version is too old). They could perfectly well use the library with any existing YAML file with no problems. Another case where I don't want to introduce another build-time requirement is for packaging.

Maybe if we put the check in SConstruct, it could be a warning for the build/install stage and an error during the test stage?

speth · 2020-07-05T14:34:57Z

Hm, I didn't realize that we didn't run the tests in the packaging environments. But is there any environment we have set up where installing ruamel.yaml is any more difficult than installing Numpy or Cython? I feel like people usually ignore warnings that pop up in scons build (which isn't unreasonable in 10000 lines of output).

Despite what I said earlier, maybe we should check for the minimum ruamel.yaml version in the conversion scripts, given that we know that there are still-supported operating systems which provide older versions. And it would be nice to provide a more helpful error message than AttributeError: module 'ruamel.yaml' has no attribute 'RoundTripRepresenter'.

At that point, checking for ruamel.yaml at the scons test stage isn't much different from just letting those particular tests fail. I think the point of checking at the start of scons build is to fail fast if possible.

bryanwweber · 2020-07-07T17:39:09Z

Thanks for the thoughts @speth. I implemented the version check in the *2yaml.py scripts for now, to improve the error message as you say.

Hm, I didn't realize that we didn't run the tests in the packaging environments.

It depends on the package, but for instance, for the macOS MATLAB package, we don't build the Python module at all, so there's no reason to have ruamel.yaml installed.

But is there any environment we have set up where installing ruamel.yaml is any more difficult than installing Numpy or Cython?

Not particularly, at least, not that I can think of. Apologies for being dense, I probably should have caught this before, but are you suggesting that the check for ruamel.yaml only be done if python_package != 'none'? I have a few conflicting thoughts here, let me write them all out and see if things start to clarify.

First, think that conditioning the check on actually building the Python interface could make sense. On the other hand, I have a gut feeling that we shouldn't restrict the user's environment unless it's really necessary.

What if we check for a good version if a user tries to install the Python interface? A user running scons install will presumably want to actually use the interface, so that'd be a good time to check without restricting the build step. And fixing the error wouldn't require rebuilding anything, so it wouldn't be a case where the user would save time by seeing the error in the build step.

bryanwweber · 2020-07-07T21:29:28Z

OK, I implemented a check of ruamel.yaml at install time. I also slightly redid the check for Python version to make the required version for both the full and minimal interfaces the same. I'm not sure about this change, because it's possible in the future that the minimum version for the minimal interface could be lower than for the full interface (which is how it is implemented right now). So thoughts about that change would be appreciated, and its in its own commit so can be dropped easily.

SConstruct

speth · 2020-07-10T02:15:02Z

are you suggesting that the check for ruamel.yaml only be done if python_package != 'none'? I have a few conflicting thoughts here, let me write them all out and see if things start to clarify.

Yes, that was my thought. If you aren't installing at least the "minimal" Python module, it doesn't matter whether or not you have ruamel.yaml installed.

First, think that conditioning the check on actually building the Python interface could make sense. On the other hand, I have a gut feeling that we shouldn't restrict the user's environment unless it's really necessary.

I'm confused - what does "restricting the user's environment" mean?

What if we check for a good version if a user tries to install the Python interface? A user running scons install will presumably want to actually use the interface, so that'd be a good time to check without restricting the build step. And fixing the error wouldn't require rebuilding anything, so it wouldn't be a case where the user would save time by seeing the error in the build step.

Checking before install or test makes sense. Currently, you're only doing it before install, right?

bryanwweber · 2020-07-10T11:04:03Z

what does "restricting the user's environment" mean?

Requiring them to install ruamel.yaml when they don't need it.

Checking before install or test makes sense. Currently, you're only doing it before install, right?

In SConstruct, the only check is before install. The test phase is de facto handled by the checks in the scripts themselves. It would be possible to expand the SConstruct check to include the test phase, but that would preclude running subsets of the test suite (for example, scons test-thermo) if the Python interface had been built - perhaps a minor consideration, but since it isn't necessary to check at test time in SConstruct given the check in the script, I don't think its necessary to add it.

speth · 2020-07-10T13:27:18Z

If I'm running scons test, I'd rather it fail immediately than run for several minutes before telling me that I need to install ruamel.yaml. What about writing something like:

check_for_ruamel_yaml = any(target in COMMAND_LINE_TARGETS for target in ['install', 'test', 'test-python-convert'])

and then using this variable in the various if statements? I think this catches all of the cases where you would need ruamel.yaml.

bryanwweber · 2020-07-10T18:29:35Z

@speth I implemented that suggestion, thanks. Any thoughts about synchronizing the minimum Python versions?

speth

I think this looks good, generally. Just had a couple of minor suggestions.

SConstruct

interfaces/cython/cantera/ck2yaml.py

To run the YAML converter scripts, ruamel.yaml must be installed with a minumum version at least 0.15.34. We aren't sure what the true minimum version is, but 0.15.34 was released in September 2017 and is the newest version available from the Ubuntu 18.04 repositories.

One of the SolutionArray tests uses pandas as an output option.

Check for the minimum ruamel.yaml version, 0.15.34, at install and test time. The check happens at install and test time because ruamel.yaml is only required to run the Python interface, not to build it.

The minimum Python version is the same for the full and the minimal interfaces.

The minimum NumPy version is 1.12 to support the use of numpy.full() in the SolutionArray interface introduced in Cantera#838.

Since we set the minimum Python version in SConstruct, use that value to fill in python_requires in setup.py.in. This ensures that when the minimum Python version is changed in the future, setup.py will automatically reflect the change.

bryanwweber · 2020-07-23T19:04:27Z

@speth Thanks for the comments! Updated now.

speth

Thanks! This looks good now.

bryanwweber mentioned this pull request Jul 2, 2020

Insufficient ruamel.yaml dependencies on older systems #831

Closed

bryanwweber force-pushed the fix-831 branch from a0dfee9 to 35507d1 Compare July 2, 2020 19:58

ischoegl reviewed Jul 2, 2020

View reviewed changes

bryanwweber force-pushed the fix-831 branch 2 times, most recently from 243f9ff to b876598 Compare July 3, 2020 11:42

speth reviewed Jul 3, 2020

View reviewed changes

bryanwweber force-pushed the fix-831 branch from b876598 to 0a90f9c Compare July 7, 2020 17:10

bryanwweber force-pushed the fix-831 branch from 0a90f9c to bb593b3 Compare July 7, 2020 21:27

ischoegl mentioned this pull request Jul 9, 2020

Fixes Indexed assignment of extra columns in SolutionArray #838

Merged

4 tasks

ischoegl reviewed Jul 9, 2020

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

bryanwweber force-pushed the fix-831 branch from bb593b3 to db04d7b Compare July 10, 2020 18:28

speth reviewed Jul 22, 2020

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

SConstruct Show resolved Hide resolved

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

bryanwweber added 6 commits July 23, 2020 12:34

[CI] Install pandas on CI for testing

7c6cbae

One of the SolutionArray tests uses pandas as an output option.

[SCons] Check for minimum ruamel.yaml version

2d2f358

Check for the minimum ruamel.yaml version, 0.15.34, at install and test time. The check happens at install and test time because ruamel.yaml is only required to run the Python interface, not to build it.

[SCons] Synchronize minimum Python versions

9554d25

The minimum Python version is the same for the full and the minimal interfaces.

[Cython] Enforce a minimum NumPy version

46d8ff7

The minimum NumPy version is 1.12 to support the use of numpy.full() in the SolutionArray interface introduced in Cantera#838.

[Python] Single-source the minimum Python version

16c8fe6

Since we set the minimum Python version in SConstruct, use that value to fill in python_requires in setup.py.in. This ensures that when the minimum Python version is changed in the future, setup.py will automatically reflect the change.

bryanwweber force-pushed the fix-831 branch from 5209ed6 to 16c8fe6 Compare July 23, 2020 19:03

speth approved these changes Jul 27, 2020

View reviewed changes

speth merged commit ba13c65 into Cantera:main Jul 27, 2020

bryanwweber deleted the fix-831 branch July 27, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SCons] Check for minimum version of ruamel.yaml #891

[SCons] Check for minimum version of ruamel.yaml #891

bryanwweber commented Jul 2, 2020

ischoegl Jul 2, 2020

ischoegl Jul 2, 2020 •

edited

Loading

ischoegl Jul 2, 2020 •

edited

Loading

bryanwweber Jul 2, 2020

codecov bot commented Jul 2, 2020 •

edited

Loading

speth left a comment

bryanwweber commented Jul 3, 2020

speth commented Jul 5, 2020

bryanwweber commented Jul 7, 2020

bryanwweber commented Jul 7, 2020

speth commented Jul 10, 2020

bryanwweber commented Jul 10, 2020

speth commented Jul 10, 2020

bryanwweber commented Jul 10, 2020

speth left a comment

bryanwweber commented Jul 23, 2020

speth left a comment

[SCons] Check for minimum version of ruamel.yaml #891

[SCons] Check for minimum version of ruamel.yaml #891

Conversation

bryanwweber commented Jul 2, 2020

ischoegl Jul 2, 2020

Choose a reason for hiding this comment

ischoegl Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

ischoegl Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

bryanwweber Jul 2, 2020

Choose a reason for hiding this comment

codecov bot commented Jul 2, 2020 • edited Loading

Codecov Report

speth left a comment

Choose a reason for hiding this comment

bryanwweber commented Jul 3, 2020

speth commented Jul 5, 2020

bryanwweber commented Jul 7, 2020

bryanwweber commented Jul 7, 2020

speth commented Jul 10, 2020

bryanwweber commented Jul 10, 2020

speth commented Jul 10, 2020

bryanwweber commented Jul 10, 2020

speth left a comment

Choose a reason for hiding this comment

bryanwweber commented Jul 23, 2020

speth left a comment

Choose a reason for hiding this comment

ischoegl Jul 2, 2020 •

edited

Loading

ischoegl Jul 2, 2020 •

edited

Loading

codecov bot commented Jul 2, 2020 •

edited

Loading