Simplify/Refactor buildutils.py

[bryanwweber]

Changes proposed in this pull request

• Simplify/Refactor the site_scons/buildutils.py file. As mentioned in the discussion post, this is going to be a long-running project. I'm planning to do the scripts/files in batches, so buildutils.py was the first.

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]

🎉 ... as I mentioned elsewhere, this is not necessarily my strength. From what I can tell, this is largely to improve formatting, with a couple of more substantial changes. Big 👍 for logger, but os.path is very much alive. Snake case, quotes, etc. are as you mentioned elsewhere. Would you mind commenting on other parts you tried to tackle in this PR? Besides build_utils.py of course ...

[bryanwweber]

Thanks @ischoegl!

• My goal in general is to simplify the code, in part by using Python 3.6 and up features (f-strings and pathlib are the two that come immediately to mind). I'm also trying to update the code style to reflect modern Python practices and PEP-8. I guess there wasn't a whole lot of simplification that was able to be done to buildutils.py, but I think the changes here will make it easier to simplify other things later, or possibly add features. This is really the tip of the iceberg, so to speak.
• The changes in the SCons* files are almost entirely due to changing/removing functions in buildutils.py. There are a few places where there are unrelated changes to the SCons* files, but I think they're pretty small changes for now that I made incidentally while making other changes.
• I'm a big fan of pathlib and a big anti-fan of os.path. Operating purely on strings when it comes to paths is (can be) really tricky, especially when working between platforms. pathlib automatically handles things like different slash conventions, trailing slashes, etc. (Edit to add: the intention with this PR was not to replace os.path everywhere, yet. I'm going to go through the files in batches to simplify PR review).

[ischoegl]

No objections from my side - the CI appears to be happy, so I am happy (I don't have enough insights to go beyond).

PS: Regarding my comment about os.path - I noticed a couple of new imports for the 'un'preferred library, hence my comment. ... overall I second your preference for pathlib, but it appears the transition will come later, correct? From personal experience, replacing paths is laborious, and the diff is already quite busy because of the formatting changes. So leaving it for later is likely preferable.

[bryanwweber]

the CI appears to be happy, so I am happy (I don't have enough insights to go beyond).

Thanks @ischoegl If there are any places where you see possible improvements in the Python, I'd be happy to know them!

I noticed a couple of new imports for the 'un'preferred library, hence my comment. ... So leaving it for later is likely preferable.

Yes, that's my plan 😄 I started here by limiting the imports that from buildutils import * takes, which meant that all the stdlib imports in buildutils that were carried into other files were removed. And then did the minimum necessary in each of the SCons* files to get them working again.

I should also say that I'm trying to keep exact feature parity as I make these changes, including exactly the same output as before. So far, I've been successful 😃

[ischoegl]

Alright 😉 ... I looked over buildutils.py and have a couple of minor points. Can give it a closer look later on.

Understood about from xyz import * ... linters really don't like it.

[ischoegl]

While I believe the Python code overall looks good, some of the docstrings could use improvements. Also, you started applying PEP 484, but didn't follow through with all signatures. While I know that some don't like the appearance, I find those annotations tremendously helpful even when looking at code that I wrote myself (especially once short-term memory no longer kicks in).

[ischoegl]

@bryanwweber ... feel free to consider all of my comments as resolved. I'll leave a final approval to those with more intricate knowledge of the internal workings of scons (myself, I'd have to rely on GH Actions passing). Apart from that, I am pretty happy that I can make more sense of buildutils.py after the refactoring.

[speth]

I think most of my in-line comments are fairly minor.

The one slightly larger concern I have is that I don't really see the benefit to using the logging module here. I don't think it's possible to use any of this except when run as a script by SCons, so there's not really a case where this would need to interact with other logging activities, which is where the more complex logging machinery makes sense. This takes a chunk of additional setup in build_utils.py, plus has several end-use points where you are forced to do convoluted things like logger.info(__doc__, extra={"print_level": False}) instead of the simple print(__doc__).

[bryanwweber]

The one slightly larger concern I have is that I don't really see the benefit to using the logging module here. I don't think it's possible to use any of this except when run as a script by SCons, so there's not really a case where this would need to interact with other logging activities, which is where the more complex logging machinery makes sense. This takes a chunk of additional setup in build_utils.py, plus has several end-use points where you are forced to do convoluted things like logger.info(__doc__, extra={"print_level": False}) instead of the simple print(__doc__).

Are you trying to get rid of all usage of print? Why?

@speth I considered a few things in choosing the logging module here. I'm not totally sold that it's the right choice, so I'd be happy to discuss. My reasoning was:

1. The logging module allows us (as developers) to communicate intent by the method name instead of with the first few words of the message.
2. It makes it easier to be consistent with message formatting. info messages will always say INFO unless you explicitly disable that, likewise warning, error, and debug. Combined with the first reason above, this means no more forgetting to add context about how serious a problem you think this output is.
3. It should allow users to have finer control over the output. SCons is really noisy IMO and our configuration and various other output doesn't help that. It'd be nice to have a setting for "Yes, I'm pretty sure everything is going to work, just give me a progress bar" or even no output at all. I think it will be possible, with logging, for a user to set a variable in cantera.conf (or on the command line) and select the level of output, and I don't think that'd be possible (or at least much more difficult) just by using print calls.
4. It's more of a "set-it-and-forget-it" approach to output, in terms of where the output is sent. Right now, I've configured it so that debug, info, and warning output is sent to stdout and error and higher are sent to stderr. Using print calls would require setting the file kwarg for error messages if we want to send error output to stderr (which I believe would be an improvement).

Now, the counterarguments (which you've noted, and which I also thought of in working on this and balanced in favor of logging):

1. It's a lot of configuration.
2. One of the big advantages of logging is collecting messages from long-running processes when you need the program to keep running, but want a log of any output; or, interacting with/combining several libraries and combining their output. Obviously, as you note, SCons scripts are basically standalone, run-once scripts, so these advantages are moot.
3. The convoluted method of printing "regular" output instead of log messages, via the print_level Boolean.

So, those are the thoughts that I had. I did consider a couple of other options for terminal output as well... One was, if you look at the commit history, I'm pretty sure it's still there where I started with two Logger instances which avoided the problem of the weird Boolean hack. On the other hand, it wasn't clear when to use each logger, so I didn't like it ☹️

I similarly thought about using a Logger for INFO/WARNING/ERROR messages, and print() for "output", but again, it wasn't clear to me how I would explain to someone when to use which, so I didn't do it. I suppose there's a similar problem with "when to use print_level=False, but at least that's self-documenting.

Let me know what you think! Like I said, I'm not totally sold on using logging here, so I'd be happy to hear any further thoughts you have.

[speth]

Overall, I think I'm satisfied with the direction this has gone. There were just two minor suggested changes below.

I agree that there's some benefit to using logging.info("...") instead of print("INFO: ..."), and the print_level argument is not that bad for the handful of cases where we want to print something without a prefix.

Regarding a few of the comments, I'm not convinced that trying to reduce the verbosity of the build system would be productive. There are just too many cases where the details of the build log are the only thing that helps identify a problem. But this PR doesn't actually move in that direction, so this is kind of a moot point.