-
Notifications
You must be signed in to change notification settings - Fork 3
Adopt central API interface #644
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
Conversation
Following the [decision for unifying programming interfaces](https://github.com/pyiron/decisions/blob/main/decisions/25-001-split-api.md) within the pyiron project.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
WalkthroughA new module, Changes
Sequence Diagram(s)sequenceDiagram
participant ExternalLibrary
participant executorlib.api
participant InternalModules
ExternalLibrary->>executorlib.api: import (e.g. get_command_path, SocketInterface)
executorlib.api->>InternalModules: Re-export selected functions/classes
ExternalLibrary->>executorlib.api: Use stable API functions/classes
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #644 +/- ##
==========================================
- Coverage 96.83% 96.81% -0.02%
==========================================
Files 30 29 -1
Lines 1294 1288 -6
==========================================
- Hits 1253 1247 -6
Misses 41 41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
notebooks/4-developer.ipynb (1)
132-132: Minor: Improve consistency and hyperlinking.Consider adding backticks or hyperlinks around
executorlib.api(similar to earlier module references) and rephrase slightly for clarity and flow. For example:- It is not recommended to import components from other parts of executorlib in other libraries, only the interfaces in `executorlib` and `executorlib.api` are designed to be stable. + It is not recommended to import components from other parts of executorlib in external projects; only the interfaces in `executorlib` and `executorlib.api` are guaranteed to be stable.executorlib/api.py (2)
1-6: Fix docstring grammar and spelling.There are typos and awkward phrasing (e.g., "inteface" → "interface" and missing punctuation). Suggest this diff:
- External application programming interface (API) following the semantic versioning this inteface is promised to remain - stable during minor releases and any change in the interface leads to a major version bump. External libraries should - only use the functionality in this API in combination with the user interface defined in the root __init__.py, all other - functionality is considered internal and might change during minor releases. + External application programming interface (API) following semantic versioning; this interface is promised to remain + stable during minor releases, and any change to its contract will trigger a major version bump. External libraries + should only use the functionality in this API (and the user interface in the root `__init__.py`); all other + components are internal and may change without notice.
21-33: Explicit__all__defines the public API.Listing the exports here provides a clear contract for external consumers. Consider the following optional enhancements:
- Alphabetize the entries for easier maintenance.
- Introduce a
__version__attribute in this module to track the API schema version explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
executorlib/api.py(1 hunks)executorlib/standalone/__init__.py(0 hunks)notebooks/4-developer.ipynb(2 hunks)
💤 Files with no reviewable changes (1)
- executorlib/standalone/init.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: notebooks
- GitHub Check: notebooks_integration
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_win
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_flux_openmpi
🔇 Additional comments (2)
notebooks/4-developer.ipynb (1)
119-119: Documentation updated to reflect new stable API module.The reference to
executorlib.apicorrectly aligns with the new external interface. Please ensure all other occurrences in the notebook (e.g., import statements in code cells) are updated accordingly.executorlib/api.py (1)
8-19: Explicit imports reflect the stable API surface.The module cleanly imports and re-exports only the intended functions and classes from internal submodules, which aligns well with the goal of a stable, semantically versioned API.
Following the decision for unifying programming interfaces within the pyiron project.
Summary by CodeRabbit
New Features
Documentation
Refactor