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

Rework library interfaces #201

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Rework library interfaces #201

wants to merge 48 commits into from

Conversation

N-Maas
Copy link
Collaborator

@N-Maas N-Maas commented Dec 19, 2024

Effectively a complete rework of the Python and C library interfaces. It is certainly a larger change than I initially thought it would be 😄

Goals are proper error handling, making the interfaces easier and less error-prone to use, extending the API, and making the behavior of the two interfaces more similar while reducing code duplication.

Within these constraints, the user-facing API stays similar to the old one.

Specifically, this PR:

  • Introduces proper error handling. Previously, errors would be printed but otherwise silently ignored, which is clearly not desirable for a library. Now, the Python API raises a Python exception if an error occurs (note that pybind allows to automatically translate C++ exceptions to Python exceptions). In the C API, each function that can error returns a status code (or something like nullptr if it already has a return type) and additionally writes the error kind and error message to a struct of type mt_kahypar_error_t
  • Renames all functions in the Python API to use snake case, as is idiomatic in Python
  • Changes the internal implementation of the Python API to use type-erased structs instead of different classes for each kind of graph/hypergraph (i.e., same as the C interface). Consequently, this also adds support for the highest_quality preset
  • Provides proper iterators for the Python interface instead of the odd do_for_all_* methods
  • Massively reduces the number of cases where segfaults/UB can be caused via the Python interface. This includes changing the API so that initialize is guaranteed to be called first, specifying lifetimes so that the partitioned hypergraph does not outlive its hypergraph, and adding a bunch of size/bounds checks: size checks for the provided data when constructing a (partitioned) hypergraph or fixed vertices, and bounds checks for all methods that access nodes, edges or blocks via an ID.
    Note, the C interface also gets a few new checks, but most of the checks used in Python don't really work for the C interface. Out of bounds indices and providing wrong sizes are still UB in C.
  • Extends the C interface by a bunch of functions which allow to access node/edge specific properties, e.g., get the pins or the connectivity of a hyperedge. While the Python interface still has a few functions not available in the C interface, this mostly closes the gap
  • Moves most logic out of the Python/C specific implementations and into shared header files. Although for small functions, this does not necessarily make the code more readable, it ensures that both library interfaces behave the same
  • Updates the tests, examples and the readme accordingly

As a side note, the C interface is now actually usable with a C compiler.

Note that this PR does not implement proper input validation yet. I.e., reading an invalid input file still might cause a segfault even in the Python API. However, the PR already adjusts the API so that we can later implement runtime-configurable input validation without any kind of breaking change. This is done with a required context parameter for each function that constructs a (hyper)graph. While not necessary yet, it is an unproblematic change since the user needs to construct a context anyways and it allows to pass options that affect the IO via the context.

@N-Maas N-Maas linked an issue Dec 23, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connectivity in C API
1 participant