Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit {fmt} versions #1538

Merged
merged 6 commits into from
Jul 11, 2023
Merged

Limit {fmt} versions #1538

merged 6 commits into from
Jul 11, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jul 9, 2023

Changes proposed in this pull request

Frequent API changes in the {fmt} library have led to numerous workarounds in fmt.h. At the same time, the library is small (especially after version 7.0) and can be easily included from a git submodule. Note that the latest released {fmt} version is 10.0.

  • Limit support to {fmt} version 6.1.2 and newer (version packaged with Ubuntu 12.04 LTS), where SCons logic ignores older system libraries under the default setting.
  • Remove most workarounds in fmt.h
  • Test multiple {fmt} version in GH actions
  • Switch SolutionArray::save CSV writing to {fmt}

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

Closes #1537, closes #1540

If applicable, provide an example illustrating new features this pull request is introducing

While this is was not an objective of the PR, the SolutionArray CSV writer speed was improved significantly. For main, the benchmark is:

In [1]: cd samples/python/onedim/
/Volumes/Data/work/GitHub/cantera/samples/python/onedim

In [2]: %run ion_free_flame.py
[...]

In [3]: %timeit f.save("test.csv", overwrite=True)
3.49 ms ± 116 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Using the proposed PR (switch to {fmt}), this reduces to

In [3]: %timeit f.save("test.csv", overwrite=True)
1.95 ms ± 16 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

This could potentially be further improved by using version 7.1 fmt/os.h file output (1.52 ms) and/or the compile-time format string compilation FMT_COMPILE, but a major caveat is that the latter is not compatible with icc (beyond only being available after 7.0). Note that YAML output is significantly slower (bottlenecks are elsewhere), so the possible optimization is not worthwhile:

In [4]: %timeit f.save("test.h5", "test", overwrite=True)
6.81 ms ± 15.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [5]: %timeit f.save("test.yaml", "test", overwrite=True)
87.3 ms ± 122 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@ischoegl ischoegl force-pushed the fix-1526 branch 6 times, most recently from 6f94de6 to 586fd27 Compare July 9, 2023 20:00
@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Merging #1538 (db00fe1) into main (f89efe8) will decrease coverage by 0.03%.
The diff coverage is 80.00%.

❗ Current head db00fe1 differs from pull request most recent head 7bba1d2. Consider uploading reports for the commit 7bba1d2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1538      +/-   ##
==========================================
- Coverage   70.50%   70.47%   -0.03%     
==========================================
  Files         376      376              
  Lines       59075    59081       +6     
  Branches    21219    21222       +3     
==========================================
- Hits        41649    41636      -13     
- Misses      14348    14365      +17     
- Partials     3078     3080       +2     
Impacted Files Coverage Δ
include/cantera/base/fmt.h 66.66% <ø> (ø)
src/base/SolutionArray.cpp 78.57% <76.19%> (-0.28%) ⬇️
interfaces/cython/cantera/onedim.py 82.63% <100.00%> (ø)
src/base/AnyMap.cpp 83.26% <100.00%> (ø)
src/oneD/Boundary1D.cpp 55.39% <100.00%> (ø)
src/oneD/StFlow.cpp 82.18% <100.00%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ischoegl ischoegl force-pushed the fix-1526 branch 9 times, most recently from 9cabd85 to 51d3ce2 Compare July 9, 2023 23:32
@ischoegl ischoegl force-pushed the fix-1526 branch 3 times, most recently from db00fe1 to 484dcfe Compare July 10, 2023 03:25
@ischoegl ischoegl marked this pull request as ready for review July 10, 2023 04:42
@ischoegl ischoegl requested a review from a team July 10, 2023 04:42
@speth
Copy link
Member

speth commented Jul 10, 2023

For Cantera 3.0, I would prefer to maintain compatibility with fmt 6.1.2, which is the version available on Ubuntu 20.04 LTS. While using the submodule is reasonably convenient for users compiling on their own systems, it is highly undesirable when building Debian packages.

fmt 6.1.2 is the system package on Ubuntu 20.04 LTS
@ischoegl
Copy link
Member Author

ischoegl commented Jul 10, 2023

For Cantera 3.0, I would prefer to maintain compatibility with fmt 6.1.2, which is the version available on Ubuntu 20.04 LTS. While using the submodule is reasonably convenient for users compiling on their own systems, it is highly undesirable when building Debian packages.

Alright. I currently don't have a machine to test 6.1.2 (Apple silicon conda refuses to install fmt < 7.1), but I believe the PR by itself is worthwhile even if it does not resolve #1526 (it does fix two other unrelated issues). I set the minimum fmt version to 6.1.2, and removed code that assumed fmt 7.1 capabilities. I'd appreciate if we could merge this before tackling #1526?

PS: same on Windows 😠

Could not solve for environment specs
The following package could not be installed
└─ fmt 6.1.2**  does not exist (perhaps a typo or a missing channel).

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ischoegl. I think this makes sense. I had just a few small suggestions.

.github/workflows/main.yml Outdated Show resolved Hide resolved
include/cantera/base/fmt.h Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@ischoegl ischoegl force-pushed the fix-1526 branch 2 times, most recently from 503066c to 8c9b575 Compare July 10, 2023 20:47
@ischoegl
Copy link
Member Author

@speth ... thanks for your comments; everything should be taken care of.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

With this one, I think we still need a python-version list if we want it to run all the combinations of python-version and vs-toolset. In the latest CI run, this only ran (VS 14.1, Python 3.11, fmt 7.1) and (VS 14.3, Python 3.11, fmt 7.1).

Fixed!

@speth speth merged commit a4b0743 into Cantera:main Jul 11, 2023
@ischoegl ischoegl deleted the fix-1526 branch July 11, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

MSVC failures with fmt 7.1 Compilation failure with fmt=10.0
2 participants