Overhaul/Clean up/Refactor SConstruct/SConscript

[bryanwweber]

The SCons build scripts are currently quite an octopus. Although they aren't very fragile, in the sense that they tend to work on every platform we need them to, it's a little hard to know where to change/add options as needed. I think there's room to do a few things:

• Update the style of the various files to be consistent with modern Python style (rename functions from camelCase to snake_case, etc.)
• Use modern Python 3 features such as pathlib.Path, type hinting, etc. See also Make python3 default for build script #18
• Reorganize the main SConstruct file to clearly delineate where features are checked/added etc.

The overall goal is to accomplish this simplification without changing or dropping support for any of the current platforms/compilers we support.

I think doing this would have a few benefits. First, it will make it easier to onboard new folks who need to modify the build script. Second, it will make it easier to add new features, such as interface-specific installers (which will, in turn, make it easier to do things like have a Wheel package to install with pip). Third, it will simplify the code by using modern Python features. And fourth, it will allow developers to use modern Python tooling, such as static type checkers (which are built in to editors like VS Code).

This is a longer term project, I think. Since it's not high-priority I don't imagine being able to devote lots of dedicated time (depending on how much procrastinating I'm doing with grading). I'm opening this as an Idea to see if other people have feedback about whether this is worth it, and if so, other places where simplification can happen!

[speth]

And fourth, it will allow developers to use modern Python tooling, such as static type checkers (which are built in to editors like VS Code).

I think what these tools will all struggle with is some of the SCons magic. This includes both implicit import of various SCons classes like Environment and Configure, and especially the behavior of commands like Import('env', 'build', 'install') in the SConscript files. I guess it probably wouldn't be that hard to make Pylance or some other specific linters aware of the SCons types, but I'm really not sure how you would deal with the Import function, since there's no indication of what parent SConstruct file would be calling the current SConscript.

You can always add a vote for SCons/scons#3210.

[bryanwweber]

I think what these tools will all struggle with is some of the SCons magic.

Yeah, this is a bit unfortunate. I think we can probably get used to ignoring those specific errors, or perhaps # noqa them. Of course, if adding types everywhere makes things harder to read, that won't help either, so it will take a balance...

You can always add a vote for SCons/scons#3210.

Done! 😄

[speth]

I'm 👎 on littering code with # noqa, especially for this purpose. I'd rather figure out how to adjust the Pylance configuration to stop complaining about undefined variables in SCons*, and add that as a recommendation for developers to enable.

[bryanwweber]

In Pylance, you can set:

    "python.analysis.diagnosticSeverityOverrides": {
        "reportUndefinedVariable": "none"
    }

which disables reporting the undefined variables that come from SCons magic. The downside is that other undefined variables are no longer reported. Fortunately, it's usually pretty easy to find those by running one of the SCons commands (scons build etc.), it's not like this is a long-running Django site or something where you might hit an undefined variable that's going to take the site down.

[bryanwweber]

@speth @ischoegl @decaluwe

Bike-shedding time: What's your opinion of single vs. double quotes in Python code (for my purposes here, in the various SCons related files)?

My personal opinion is a pretty strong preference for double quotes as the default, and my plan in this pull request is to eventually change all the single quotes to double quotes (except where single quotes have to be used because of a double quote inside the string). Although I'm not advocating to apply Black to our code base, I agree with their justification for preferring double quotes whenever possible:

Why settle on double quotes? They anticipate apostrophes in English text. They match the docstring standard described in PEP 257. An empty string in double quotes ("") is impossible to confuse with a one double-quote regardless of fonts and syntax highlighting used. On top of this, double quotes for strings are consistent with C which Python interacts a lot with. Source

[bryanwweber]

Thanks for the thoughtful feedback. Without a way to automate this, it's just noise as you say. I'm fine with dropping the requirement.

[ischoegl]

I have no issue with dropping the rule. I don’t have strong feelings one way or the other. Only bad thing is that in hindsight, we are left with the worst of all outcomes: being stuck more or less in the middle of a transition with no plan to resolve inconsistent formatting.

Didn’t mean to hit a sore point when pointing this out in a review today btw.

[bryanwweber]

resolve inconsistent formatting.

I think that unless and until we find an auto-formatter that works on all our code (for example, I'm not aware of a formatter for Cython), we're just going to have to live with inconsistencies 🤷‍♂️

[ischoegl]

SCons is the most egregious example. Thankfully, it’s only a handful of people who have to live with an aesthetic disaster 😆. But it’s really not an issue worth losing sleep over.

[speth]

Thanks, both. I'm fine with leaving double quotes as a recommendation in our style guide, but just not pushing it as part of PR reviews.

[ischoegl]

Python testing ... as this is a discussion about a scons overhaul (which likely will take more than one PR), I wanted to suggest running the python test suite through pytest, which imho generates superior error stacks (when configured to provide verbose output). A significant advantage is that it becomes trivial to run individual tests:

pytest path/to/testfile.py::testModule::testName

It's fully compatible with unittest, so the churn should be manageable. The main question would be configuration of scons, where I assume that it can be done.

[bryanwweber]

I've also talked to @speth about this, it's on the long-term roadmap in my head. I think pytest offers a few other advantages, such as their fixtures and test parameterization. We've also talked about adding Hypothesis for property-based testing, to supplement the mostly regression testing we have now.

FWIW, you can run individual tests pretty easily for the existing unittest suite:

PYTHONPATH=build/python python -m unittest cantera.test.test_module[.test_class[.test_name]]

[bryanwweber]

Another advantage of Pytest is that I bet we can get that to run/manage the Googletest based test suite as well, for example: https://pypi.org/project/pytest-cpp/

[ischoegl]

True about the unittest command, but for some reason I was never able to memorize that one, and mostly didn't feel like hunting for the posts where it was mentioned (anything that starts with caps PYTHONPATH is mentally blocked!). pyenv would presumably make that unnecessary, but that's another matter.

[bryanwweber]

I use

PYTHONPATH=build/python ipython

all the time for testing changes in the Python interface. Saves having to install the interface somewhere etc. etc. I don't think I've ever posted it very many places publicly because, as you imply, PYTHONPATH can get you into trouble, but this only works for one-off commands, as opposed to exporting in the .zshrc or something, so it can't cause too much trouble 😄

[speth]

If there's a way to use pytest if it's installed but fall back to unittest, I have no objection. Otherwise, I'm not sure it's really worth adding another build dependency.

I also use PYTHONPATH=build/python all the time with either ipython or python -m unittest .... And for the gtest suite, you can use commands like scons test-general gtest_flags=--gtest_filter=AnyMap.missingKey, which is a bit awkward and has the unfortunate effect of saving the value of gtest_flags in cantera.conf even when you don't want it to. A simpler / easier to remember command for running a single test out of either the Python or gtest test suites would probably be beneficial.

[ischoegl]

Packaging ... another comment I have is about packaging. There currently are msi, macOS, conda and ubuntu packages (I may be missing a few). The first can be handled via scons msi, whereas the others cannot. I am not sure whether it is widely used, so it may be just be the 9-th arm of the octopus.

Tangent: Assuming that local installations are possible (I have not checked), a scons conda (to build a conda package) along with instructions would imho be more useful. It's probably still better to keep it outside of the main build script though.

[Edit: just noticed that interface-specific-installers are already mentioned at the top]

[bryanwweber]

There currently are msi, macOS, conda and ubuntu packages (I may be missing a few). The first can be handled via scons msi, whereas the others cannot.

I wonder if it'd make sense to go the other direction and pull scons msi out into a separate repo, along with the macOS package (essentially combine the two packaging repos we already have for those interfaces). In addition, I'm hoping to add a Wheel-based installer (upon which, we could probably deprecate the Python MSI packages).

Tangent: Assuming that local installations are possible (I have not checked), a scons conda (to build a conda package) along with instructions would imho be more useful

I think this would also be better going the other way. Doing scons conda would require shelling out to the conda build process (or figuring out their internal API), whereas adding a scons install-python would make the existing conda build scripts somewhat simpler.

[ischoegl]

I wonder if it'd make sense to go the other direction and pull scons msi out into a separate repo

👍 I guess this was my (implied) thought here.

I think this would also be better going the other way.

👍 Anything that makes things simpler is good. Maybe there's a way to create a single packaging repo, which can dispatch via GitHub actions, but also provides instructions to build msi, conda, etc locally?

[bryanwweber]

Maybe there's a way to create a single packaging repo, which can dispatch via GitHub actions, but also provides instructions to build msi, conda, etc locally?

I think that's a great idea, maybe one day 😄

[ischoegl]

A bike shed worthy of a king, I know ...

[speth]

At this point, I think the MSI installer is really only useful as a means of installing the Matlab toolbox, and I'm not sure that it's much better than just providing a .zip file that users can extract wherever they want. We also now have the cantera-matlab Conda package, which is easier to use in at least some ways (not requiring users to fiddle around with setting paths).

In principle, the MSI also provides library and header files if the user wants to compile apps and link to Cantera that way, but I'm not sure that's ever been super effective, since our MSI only has libraries for a single version of MSVC. And again, those might be just as efficiently distributed as a .zip file.

I think some of the things that we relied on having this installer for have become less relevant, with the move away from the executable ck2cti program several versions ago and less reliance on having a central location for Cantera's built-in data files with the YAML format (which no longer relies on an equivalent of the old elements.xml).

[bryanwweber]

The first pull request targeting this discussion is Cantera/cantera#1012 and is ready for review 🎉

[ischoegl]

As another comment that may fit in this context: how hard would it be to split libcantera into multiple libraries for core and applications? (similar on the Python front end.) Some users may only be interested in linking to the species, reactions, etc. portion (and YAML parsing of course!!), but may not require reactor networks, one-dimensional simulations etc..

[ischoegl]

It is certainly true that the Python API can be restructured outside of SConscript (which is, as you point out, much simpler), but I believe a clean solution would mirror what starts at the C++ level. A core C++ cantera library would be much leaner than application toolboxes (no sundials, etc.), which may be attractive for those working with OpenFOAM, or any other 3rd party application. It would be a long-term aspirational project, but I wanted to bring it up.

[bryanwweber]

It would be a long-term aspirational project, but I wanted to bring it up.

I think it'd be worth having a separate discussion topic devoted to this idea 😄

[speth]

If one of you wants to start that, I can provide some of the context for why things are the way they are -- not all of these things are as easy to split up as you might like.

[ischoegl]

Will do.

[ischoegl]

Done - see #97

[bryanwweber]

I read today about Ralf Gommers' efforts to transition SciPy to the Meson build system. His motivations are varied, but the main one was that distutils is deprecated in Python 3.10 and will be removed in 3.12, some 2 years from now, and NumPy (and hence SciPy) depend heavily on distutils. Setuptools is planning to include distutils with a cleaner API, but they haven't done so yet.

I wonder if something like this makes sense for Cantera too. We currently don't use distutils to drive the compiling like NumPy does, so we don't have to worry about the impending removal of distutils. There's also always the danger of missing corner cases that are fixed in the SCons build system, especially for platforms we don't test that often. Meson does seem very fast though, and as the basis of this discussion is the cruft in our SCons system, I thought it'd be worth considering a change of build system.

More information: https://labs.quansight.org/blog/2021/07/moving-scipy-to-meson/ scipy/scipy#13615

[speth]

I don't think speed is a very important motivation for changing Cantera's build system right now. A decent estimate of the time used by SCons is what it takes to run scons build when everything is up to date, which on my system takes around 2 seconds, compared to a couple of minutes spent actually compiling source files if you have to do a full rebuild (which no build system is going to reduce).

If Meson had been around when I initiated the transition from the old make-based build system (and a separately "maintained" Visual Studio project), I would certainly have considered it, but I don't see any killer features to motivate switching now. I'm a little surprised that SCons didn't even bear mentioning in the discussion of viable build systems for SciPy.

[bryanwweber]

I agree, those are all good points. SCons handles parallel builds where distutils doesn't, which I think was a big part of the speedup.

I also thought it was interesting that SCons didn't merit a mention, other than a related package in passing. I wonder if he was focusing on PEP517 compatible builders?

I think one other feature that Meson has that might be useful is it's intentionally not very extensible. If it can do what we need without crazy hack-arounds, that would make the system easier to maintain. That's a big "if" though, and it would probably take more than a little work to figure out where the problems are.