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

Experimental clib from doxygen #1835

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Dec 29, 2024

Changes proposed in this pull request

This PR seeks to implement a basis for future auto-generated Cantera APIs that are fully documented based on information provided in C++ docstrings.

  • New sourcegen scaffolder generates an experimental clib version ("clib-experimental")
  • The clib scaffolder is based on doxygen tag files and associated XML anchorfiles
  • A proposed minimal YAML syntax is used to reference C++ functions and methods (for details, see file interfaces/sourcegen/sourcegen/_data/README.md)
  • A portion of clib googletests is adopted for illustration and code coverage as test-clib-experimental
  • Partial CI is added to the clang-compiler action
  • API information is obtained without having to parse pre-existing CLib headers (i.e. the original approach for the .NET API)

This proof of concept re-implements approaches of the traditional CLib library and is newr feature-complete, although the conversion of zeroD and oneD objects is currently omitted. One other currently omitted feature involves the inclusion of code that is difficult to automate, although it is expected to be rare; in many instances, tweaks to the C++ interface may be the go-to approach instead. Edit: custom code is now supported.

The configuration involves YAML markup that only uses minimal information for each entry, e.g.

- name: newSolution
  implements: newSolution(const string&, const string&, const string&)
  uses: [thermo, kinetics, transport]
- name: name

which produces fully documented declarations

/**
 *  Create and initialize a new Solution manager from an input file.
 *
 *  @param infile       name of the input file
 *  @param name         name of the phase in the file. If this is blank, the first phase in the file is used.
 *  @param transport    name of the transport model.
 *  @returns            Handle to stored Solution object or -1 for exception handling.
 *
 *  @implements constructor: shared_ptr<Solution> newSolution(const string&, const string&, const string&)
 *  @relates Solution::thermo(), Solution::kinetics(), Solution::transport()
 */
int sol3_newSolution(const char* infile, const char* name, const char* transport);

/**
 *  Return the name of this Solution object.
 *
 *  @param handle       Handle to queried Solution object.
 *  @param[in] bufLen   Length of reserved array.
 *  @param[out] buf     Returned string value.
 *  @returns            Actual length of string or -1 for exception handling.
 *
 *  @implements getter: string Solution::name()
 */
int sol3_name(int handle, int bufLen, char* buf);
and implementations (click to expand)
int sol3_newSolution(const char* infile, const char* name, const char* transport)
{
    // constructor: shared_ptr<Solution> newSolution(const string&, const string&, const string&)
    try {
        auto obj = newSolution(infile, name, transport);
        int id = SolutionCabinet::add(obj);
        // add all associated objects
        if (obj->thermo()) {
            ThermoPhaseCabinet::add(obj->thermo(), id);
        }
        if (obj->kinetics()) {
            KineticsCabinet::add(obj->kinetics(), id);
        }
        if (obj->transport()) {
            TransportCabinet::add(obj->transport(), id);
        }
        return id;
    } catch (...) {
        return handleAllExceptions(-2, ERR);
    }
}

int sol3_name(int handle, int bufLen, char* buf)
{
    // getter: string Solution::name()
    try {
        string out = SolutionCabinet::at(handle)->name();
        copyString(out, buf, bufLen);
        return int(out.size()) + 1;
    } catch (...) {
        return handleAllExceptions(-1, ERR);
    }
}

Here is an example for a full YAML input used for code generation

sol3_auto.yaml (click to expand)
# Auto-generated CLib API for %Cantera's Solution class.
# Partially implements a replacement for CLib's traditional @c ct library.

# This file is part of Cantera. See License.txt in the top-level directory or
# at https://cantera.org/license.txt for license and copyright information.

prefix: sol3
base: Solution
parents: []  # List of parent classes
derived: [Interface]  # List of specializations
recipes:
- name: newSolution
  implements: newSolution(const string&, const string&, const string&)
  uses: [thermo, kinetics, transport]
- name: newInterface
  implements:
    newInterface(const string&, const string&, const vector<shared_ptr<Solution>>&)
  uses: [thermo, kinetics]
- name: del
  uses: [thermo, kinetics, transport]
- name: name
- name: setName
- name: thermo
- name: kinetics
- name: transport
- name: setTransportModel
  code: |-
    try {
        auto obj = SolutionCabinet::at(handle);
        TransportCabinet::del(
            TransportCabinet::index(*(obj->transport()), handle));
        obj->setTransportModel(model);
        return TransportCabinet::add(obj->transport(), handle);
    } catch (...) {
        return handleAllExceptions(-2, ERR);
    }
- name: nAdjacent
- name: adjacent
  implements: Solution::adjacent(size_t)
  uses: [thermo, kinetics, transport]
  what: constructor  # registers object in CLib storage
# - name: adjacentName
- name: cabinetSize

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

See Cantera/enhancements#39

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

Run the following command from the Cantera root folder:

scons doxygen
python3 interfaces/sourcegen/run.py --api=clib --output=.
scons build clib_experimental=y

A partial test suite based on the traditional CLib ensures that code performs as expected:

scons test-clib-experimental

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

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Thanks Ingmar! I know this is still in draft but I wanted to leave a small comment that might be intrusive later

interfaces/sourcegen/sourcegen/_HeaderFileParser.py Outdated Show resolved Hide resolved
interfaces/sourcegen/sourcegen/_TagFileParser.py Outdated Show resolved Hide resolved
@ischoegl ischoegl force-pushed the sourcegen-doxygen-clib branch 5 times, most recently from 9f3f308 to 17b2c98 Compare January 2, 2025 00:21
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.41%. Comparing base (2e0d8ac) to head (19ca9a4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1835   +/-   ##
=======================================
  Coverage   74.41%   74.41%           
=======================================
  Files         382      382           
  Lines       53411    53411           
  Branches     9026     9026           
=======================================
  Hits        39747    39747           
  Misses      10617    10617           
  Partials     3047     3047           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ischoegl ischoegl force-pushed the sourcegen-doxygen-clib branch from 22be9f9 to a06ec48 Compare January 3, 2025 04:41
@ischoegl ischoegl marked this pull request as ready for review January 3, 2025 05:05
@ischoegl
Copy link
Member Author

ischoegl commented Jan 3, 2025

@speth and @bryanwweber ... this is ready for a review.

As an aside, there is a minor caveat for the .NET workflow on Windows, where a Python 3.10 version apparently does not respect PEP 604. I'll address this together with other comments. Edit: nevermind ... windows-2022 appears to use an older system Python and my initial attempt to override failed.

@ischoegl ischoegl requested a review from a team January 3, 2025 05:09
@ischoegl ischoegl force-pushed the sourcegen-doxygen-clib branch from a06ec48 to 97bfa50 Compare January 3, 2025 05:16
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Thanks for all this work Ingmar! Nearly all of my comments here are suggestions to improve the type hinting. In some cases, you'll need to add an import (I think just for Iterable). Obviously these don't affect the runtime, but my thought was that if we're going to have type hints they should at least be complete 😄

interfaces/sourcegen/sourcegen/_HeaderFileParser.py Outdated Show resolved Hide resolved
interfaces/sourcegen/sourcegen/_HeaderFileParser.py Outdated Show resolved Hide resolved
interfaces/sourcegen/sourcegen/_HeaderFileParser.py Outdated Show resolved Hide resolved
interfaces/sourcegen/sourcegen/_HeaderFileParser.py Outdated Show resolved Hide resolved
interfaces/sourcegen/sourcegen/_HeaderFileParser.py Outdated Show resolved Hide resolved
test/clib_experimental/test_clib3.cpp Outdated Show resolved Hide resolved
Also fix issues caused by differences of doxygen and Cantera whitespace
conventions in argument lists and add recipes for functions used in
googletests.
@ischoegl ischoegl force-pushed the sourcegen-doxygen-clib branch from 97bfa50 to 2350a3d Compare January 4, 2025 03:30
@ischoegl
Copy link
Member Author

ischoegl commented Jan 4, 2025

Thanks for all this work Ingmar! Nearly all of my comments here are suggestions to improve the type hinting. In some cases, you'll need to add an import (I think just for Iterable). Obviously these don't affect the runtime, but my thought was that if we're going to have type hints they should at least be complete 😄

Thanks, @bryanwweber, for the prompt review! I agree that having done most of the type hints already, it makes sense to take it all the way. I adopted almost all of the suggestions directly and added some conversions, so the googletest suite is almost fully comparable to the traditional CLib version for the parts that I have converted..

@ischoegl
Copy link
Member Author

ischoegl commented Jan 4, 2025

@bryanwweber and @speth ... the PR ready for further comments/reviews. PS: had to fix some unexpected CI failures. / PPS: decided to include support for custom code to be nearly feature-complete.

@ischoegl ischoegl force-pushed the sourcegen-doxygen-clib branch 2 times, most recently from 639916f to 0a976c1 Compare January 5, 2025 02:03
@ischoegl ischoegl force-pushed the sourcegen-doxygen-clib branch from 0a976c1 to 19ca9a4 Compare January 5, 2025 02:05
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Thanks Ingmar! The Python looks good to me 😄 I'm not that familiar with the overall use case though as I've been out of the loop with these changes, so I assume that it works the way we want and anyways we're early in the 3.2 cycle so we can always fix it 😃

@ischoegl
Copy link
Member Author

ischoegl commented Jan 5, 2025

Thanks Ingmar! The Python looks good to me 😄 I'm not that familiar with the overall use case though as I've been out of the loop with these changes, so I assume that it works the way we want and anyways we're early in the 3.2 cycle so we can always fix it 😃

Thanks, @bryanwweber! The overall use case is Cantera/enhancements#39 (specifically, a drop-in replacement for the existing clib that won't require manual coding). I'll wait for @speth to chime in.

@ischoegl ischoegl requested a review from speth January 5, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants