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.

Sign up for GitHub

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

Jump to bottom

Add hybrid plugin system notebook #99

Merged
merged 2 commits into from
Sep 25, 2024
Merged

Add hybrid plugin system notebook #99

merged 2 commits into from
Sep 25, 2024

Conversation

feltech
Copy link
Member

@feltech feltech commented Jul 16, 2024 •
edited
Loading

Add notebook for the design, documentation and e2e testing of the Hybrid Plugin System introduced in OpenAssetIO/OpenAssetIO#1202

Current rendered version

Notebook Driven Development 😉

  • Design API by documentation / example sketches (notebook cannot be executed at this stage)
  • Verify locally by executing notebook against proposed upstream changes, making tweaks to the design as necessary.
  • Test final released upstream changes against the notebook as part of CI.
  • Document by tidying / expanding the notebook to become fully-fledged documentation.

@feltech feltech self-assigned this Jul 16, 2024
This was referenced Jul 16, 2024
Merged
Merged
@feltech feltech force-pushed the work/OpenAssetIO/1202-hybridPluginSystem branch 2 times, most recently from 8fb3600 to 7949ef5 Compare July 16, 2024 14:44
@feltech feltech mentioned this pull request Jul 16, 2024
Merged
2 tasks
@feltech feltech changed the title [Docs] Add hybrid plugin system notebook Add hybrid plugin system notebook Jul 22, 2024
@feltech feltech mentioned this pull request Aug 29, 2024
Open
@feltech feltech force-pushed the work/OpenAssetIO/1202-hybridPluginSystem branch 2 times, most recently from b2c830e to f3f929f Compare August 30, 2024 17:22
@feltech
[CI] Pin built OpenAssetIO to mirror PyPI version
87f8b91 
Unit tests and Jupyter notebooks use the latest release of OpenAssetIO
(see `examples/resources/requirements.txt` and `pyproject.toml`).

However, C++ tests, which currently must build OpenAssetIO from source,
were using the latest `main`.

So ensure we test against a consistent version of OpenAssetIO for both
C++ and Python tests by pinning the checkout of OpenAssetIO to the
latest release. Note that we cannot specify a semver range, so this
will get out of date on the next release.

Future CI work is required to design a more well-thought testing policy
e.g. testing against supported version ranges vs. `main` and nightlies.

Signed-off-by: David Feltell <[email protected]>
@feltech feltech force-pushed the work/OpenAssetIO/1202-hybridPluginSystem branch from f3f929f to 044c31f Compare August 30, 2024 17:59
@feltech feltech marked this pull request as ready for review August 30, 2024 17:59
@feltech feltech requested a review from a team as a code owner August 30, 2024 17:59
@feltech feltech force-pushed the work/OpenAssetIO/1202-hybridPluginSystem branch 4 times, most recently from 7131c51 to f17f72c Compare August 30, 2024 18:25
foundry-markf
foundry-markf approved these changes Sep 2, 2024
View reviewed changes
Copy link

@foundry-markf foundry-markf left a comment

Choose a reason for hiding this comment

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

Some minor questions, but if I'm reading the code correctly, the code in these notebooks get run in the CI, so they _have to be correct or the CI will fail?

examples/hybrid_plugin_system.ipynb Show resolved Hide resolved
examples/resources/hybrid_plugin_system/openassetio_config.toml Outdated Show resolved Hide resolved
tomc-cinesite
tomc-cinesite suggested changes Sep 2, 2024
View reviewed changes
Copy link

@tomc-cinesite tomc-cinesite left a comment

Choose a reason for hiding this comment

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

Awesome to see this ❤️ this is going to be mega.

Sorry a little short on time, so fairly high-level comments and written somewhat hurriedly so please forgive any brevity!

Mostly wondering if it might be worth keeping focused on the common use case vs technically how it works at a lower level. I know the API has quite a steep learning curve for people already, so being more practical-outcome based instead of covering the basic mechanism might help less experience OpenAssetIO-ers get up to speed?

examples/hybrid_plugin_system.ipynb Outdated Show resolved Hide resolved
examples/hybrid_plugin_system.ipynb Show resolved Hide resolved
examples/hybrid_plugin_system.ipynb Outdated Show resolved Hide resolved
examples/hybrid_plugin_system.ipynb Outdated Show resolved Hide resolved
@feltech feltech mentioned this pull request Sep 2, 2024
Open
@feltech
Copy link
Member Author

feltech commented Sep 2, 2024 •
edited
Loading

Some minor questions, but if I'm reading the code correctly, the code in these notebooks get run in the CI, so they _have to be correct or the CI will fail?

Yeah exactly. Unfortunately, since we don't publish SimpleCppManager, this notebook only runs in CI on Linux (Docker). I added OpenAssetIO/OpenAssetIO#1408 as a consequence.

foundry-markf
foundry-markf approved these changes Sep 3, 2024
View reviewed changes
Copy link

@foundry-markf foundry-markf left a comment

Choose a reason for hiding this comment

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

I read the changes in the commits mentioned in the threads, and I'm fine with these. Can't speak on behalf of Tom, but I'll put my approval on this.

tomc-cinesite
tomc-cinesite reviewed Sep 3, 2024
View reviewed changes
Copy link

@tomc-cinesite tomc-cinesite 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 the tweaks @feltech 🙏 - sorry I cant close that comment.

examples/hybrid_plugin_system.ipynb Outdated Show resolved Hide resolved
examples/hybrid_plugin_system.ipynb Outdated Show resolved Hide resolved
@feltech
Copy link
Member Author

feltech commented Sep 17, 2024

Thanks for the tweaks @feltech 🙏 - sorry I cant close that comment.

I struggle to find a setting in GitHub to allow users to resolve their own comments 😞 . Perhaps a 👍 is sufficient for now to signal you're happy for the comment to be resolved?

@tomc-cinesite
Copy link

Thanks for the tweaks @feltech 🙏 - sorry I cant close that comment.

I struggle to find a setting in GitHub to allow users to resolve their own comments 😞 . Perhaps a 👍 is sufficient for now to signal you're happy for the comment to be resolved?

Added thumbs up -thanks!

@feltech feltech force-pushed the work/OpenAssetIO/1202-hybridPluginSystem branch 8 times, most recently from 1c5f72d to 179e1f9 Compare September 20, 2024 07:39
foundry-markf
foundry-markf approved these changes Sep 23, 2024
View reviewed changes
Copy link

@foundry-markf foundry-markf left a comment

Choose a reason for hiding this comment

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

I had prior approved, but read through the changes GitHub tells me have been made since my last review, and LGTM

@feltech
[Docs] Add hybrid plugin system notebook
0f3d4bb 
Part of OpenAssetIO/OpenAssetIO#1202. Add a Jupyter Notebook
illustrating the design and usage of the hybrid plugin system.

Bump the minimum versions of OpenAssetIO and BAL in `requirements.txt`
to support the required features for the notebook to run.

Use a custom hybrid plugin created especially for the notebook, rather
than attempting to sellotape existing managers together (e.g. BAL and
SimpleCppManager). This allows the notebook to avoid potentially
confusing readers with misleading text about configuring two completely
independent plugins to work together. For the custom plugin sources, use
linter config lifted from the main OpenAssetIO repo, with little
modification.

Signed-off-by: David Feltell <[email protected]>
@feltech feltech force-pushed the work/OpenAssetIO/1202-hybridPluginSystem branch from 669f143 to 0f3d4bb Compare September 25, 2024 10:57
@feltech feltech merged commit 9bc0b0a into OpenAssetIO:main Sep 25, 2024
21 checks passed
@feltech feltech deleted the work/OpenAssetIO/1202-hybridPluginSystem branch September 25, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfanner rfanner rfanner left review comments

@tomc-cinesite tomc-cinesite tomc-cinesite approved these changes

@foundry-markf foundry-markf foundry-markf approved these changes

@themissingcow themissingcow Awaiting requested review from themissingcow

@hutchinson hutchinson Awaiting requested review from hutchinson

@SamCrooksFoundry SamCrooksFoundry Awaiting requested review from SamCrooksFoundry

Assignees

@feltech feltech

Labels
None yet
Projects
OpenAssetIO Development
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@feltech @tomc-cinesite @themissingcow @rfanner @foundry-markf