[Build] Generate traits rather than hard-code them #15

elliotcmorris · 2022-11-25T12:01:22Z

Closes #10

Remove hand rolled traits, and replace them with generated ones via openassetio-traitgen integration.
This is so we can avoid having to manually write and maintain custom traits, as it's too much of a maintenance burden.

This effectively unlocks further integration of mediacreation into other integrations, as traits can now be trivially added if neccesary.

Testing

The hand rolled traits had a minimal set of tests, these have been removed in favour of import tests, as we trust the traits coming out of traitgen, and we don't want to roll tests for every generated trait.
However, prior to removal, these tests were run against the generated traits, (you can see the tests were only removed in the final commit,) to verify that the traits exhibited the same behaviour before and after the swap over.

As this is a first time merge of CI cross-repo, please find the action runs on this PR targetting my fork :
elliotcmorris#2

tests/python/conftest.py

tests/python/openassetio_mediacreation/trait/test_content.py

foundrytom · 2022-11-25T14:43:06Z

~~@elliotcmorris we can delete the test code other than import checks - we went with being happy that traitgen itself is sufficiently tested.~~ Sorry just saw they were deleted in a subsequent commit. I just confoozed myself seeing the diff updating them.

foundrytom

Sorry not the most comprehensive of reviews (hopefully @feltech will be able to apply his usual 🦅 👁️ ), but looks great to me, nice one! Love how simple it is to get generation working in setup.py. Good job remembering to update README.md too! 👍

Mostly minor pedant notes.

RELEASE_NOTES.md

setup.py

RELEASE_NOTES.md

feltech

Looks good, all minor stuff.

setup.py

traits.yml

RELEASE_NOTES.md

setup.py

foundrytom

(testenv)  OpenAssetIO-MediaCreation (work/10-gen-traits) ✗ python -m pip install .
Processing /Users/tomcowland/dev/OpenAssetIO-MediaCreation
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  ERROR: Command errored out with exit status 1:
...
  Complete output (14 lines):
  error: Multiple top-level packages discovered in a flat-layout: ['ignoreme', 'decisions'].

foundrytom · 2022-11-30T18:25:46Z

[fix] Its no longer generating content traits, looks like the re-tabbing has left that dict with an extra indent level. Can we make sure we double check the CI run in the other org before merging this?

foundrytom · 2022-12-01T08:57:08Z

@elliotcmorris I've attempted to adjust the approval settings for actions here (as they don't behave as their docs say, for some reason), so maybe they'll run on the next push.

elliotcmorris · 2022-12-01T10:11:30Z

Annoying that that happened, as I installed an autoformatter to do the indentation for me, (just the redhat one that had a code extension.) I'm guessing that we don't have a yaml formatter standard as of yet.

elliotcmorris · 2022-12-01T10:31:41Z

@feltech @foundrytom

It looks like we are gonna have to do the .gitkeep/readme in an empty dir approach, explicitly defining a package (to solve the issue of it thinking decisions was the project dir,) does fail when there isn't a directory of that name in place, as we suspected.

Was wondering thoughts on doing packages in pyproject.toml vs setup.py, as pip 22 does warn

Support for `[tool.setuptools]` in `pyproject.toml` is still *beta*.

I think it's fine, this is clearly the way the wind is blowing.

foundrytom · 2022-12-01T11:10:06Z

@feltech @foundrytom

It looks like we are gonna have to do the .gitkeep/readme in an empty dir approach, explicitly defining a package (to solve the issue of it thinking decisions was the project dir,) does fail when there isn't a directory of that name in place, as we suspected.

Thanks for checking, I liked your README.md idea as it helps people work out how it actually works!

Was wondering thoughts on doing packages in pyproject.toml vs setup.py, as pip 22 does warn
Support for `[tool.setuptools]` in `pyproject.toml` is still *beta*.
I think it's fine, this is clearly the way the wind is blowing.

I'd be happy with that too, as were still in alpha.

elliotcmorris · 2022-12-01T11:14:54Z

@foundrytom @feltech
Ready for re-review, two fixups based on the issues we found
#1 elliotcmorris@95c5cd9 - Fix indentation error causing trait not to be generated
#2 elliotcmorris@7ddce56 - Add empty openassetio_mediacreation module folder, to avoid setuptools thinking decisions is the module.

CI passes on fork

foundrytom

Nice one @elliotcmorris LGTM

feltech

Minor notes, otherwise LGTM

openassetio_mediacreation/README.md

Rather than have manually maintained traits, integrate traitgen to generate the traits automatically in the build. Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

Add import tests to mediacreation, to check that the generated traits function. Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

Run pytest on PRs, so we can have some CI going. Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

Remove the tests that were testing the manually rolled traits. Now that the traits are autogenerated, we rely on the tests in openassetio-traitgen Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

Add information about the new traitgen approach to the readme. Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

JeanChristopheMorinPerso · 2022-12-04T19:43:14Z

setup.py

+
+        # Move the source trait yaml to the package directory.
+        copyfile(
+            "traits.yml", os.path.join(self.build_lib, "openassetio_mediacreation", "traits.yml")


This should be handled as package data. See https://setuptools.pypa.io/en/latest/userguide/datafiles.html

We logged an issue for this : #16

We did a short initial investigation, to see if it would "just work," but unfortunately the install was running into trouble identifying package data, I suspect related to the problems we were seeing around setuptools not figuring as as a full "package" due to generating after the fact.

But we didn't dig very far, are planning to go back later and get to it properly.

elliotcmorris self-assigned this Nov 25, 2022

elliotcmorris commented Nov 25, 2022

View reviewed changes

tests/python/conftest.py Show resolved Hide resolved

elliotcmorris commented Nov 25, 2022

View reviewed changes

tests/python/openassetio_mediacreation/trait/test_content.py Outdated Show resolved Hide resolved

elliotcmorris requested review from foundrytom and feltech November 25, 2022 12:16

elliotcmorris marked this pull request as ready for review November 25, 2022 12:16

elliotcmorris marked this pull request as draft November 25, 2022 12:17

elliotcmorris removed request for feltech and foundrytom November 25, 2022 12:17

elliotcmorris marked this pull request as ready for review November 25, 2022 12:29

elliotcmorris requested review from foundrytom and feltech November 25, 2022 12:30

foundrytom suggested changes Nov 25, 2022

View reviewed changes

RELEASE_NOTES.md Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

RELEASE_NOTES.md Outdated Show resolved Hide resolved

RELEASE_NOTES.md Show resolved Hide resolved

elliotcmorris requested a review from foundrytom November 28, 2022 13:08

feltech approved these changes Nov 28, 2022

View reviewed changes

setup.py Show resolved Hide resolved

traits.yml Outdated Show resolved Hide resolved

traits.yml Outdated Show resolved Hide resolved

traits.yml Outdated Show resolved Hide resolved

foundrytom suggested changes Nov 28, 2022

View reviewed changes

RELEASE_NOTES.md Show resolved Hide resolved

setup.py Show resolved Hide resolved

foundrytom self-requested a review November 28, 2022 18:25

elliotcmorris mentioned this pull request Nov 28, 2022

Move creation_callback arg to be final arg of generate function OpenAssetIO/OpenAssetIO-TraitGen#9

Open

elliotcmorris requested a review from feltech November 30, 2022 17:04

foundrytom suggested changes Nov 30, 2022

View reviewed changes

elliotcmorris requested a review from foundrytom December 1, 2022 11:14

foundrytom approved these changes Dec 1, 2022

View reviewed changes

feltech approved these changes Dec 1, 2022

View reviewed changes

openassetio_mediacreation/README.md Outdated Show resolved Hide resolved

openassetio_mediacreation/README.md Show resolved Hide resolved

elliotcmorris added 5 commits December 1, 2022 22:58

[Build] Generate traits

b46ad77

Rather than have manually maintained traits, integrate traitgen to generate the traits automatically in the build. Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

[Test] Add import tests

56392a0

Add import tests to mediacreation, to check that the generated traits function. Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

[CI] Run Pytest in CI

e9bac65

Run pytest on PRs, so we can have some CI going. Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

[Test] Remove Redundant Tests

a6e8fd1

Remove the tests that were testing the manually rolled traits. Now that the traits are autogenerated, we rely on the tests in openassetio-traitgen Issue #10 Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

[Docs] Update Readme project status

ae8c609

Add information about the new traitgen approach to the readme. Signed-off-by: Elliot Morris <elliot.morris@foundry.com>

elliotcmorris force-pushed the work/10-gen-traits branch from 4413021 to ae8c609 Compare December 1, 2022 22:58

elliotcmorris merged commit 0280442 into OpenAssetIO:main Dec 1, 2022

JeanChristopheMorinPerso reviewed Dec 4, 2022

View reviewed changes

elliotcmorris mentioned this pull request Dec 5, 2022

Share traits.yml via package data mechanism #16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Build] Generate traits rather than hard-code them #15

[Build] Generate traits rather than hard-code them #15

elliotcmorris commented Nov 25, 2022 •

edited

Loading

foundrytom commented Nov 25, 2022 •

edited

Loading

foundrytom left a comment

feltech left a comment

foundrytom left a comment

foundrytom commented Nov 30, 2022 •

edited

Loading

foundrytom commented Dec 1, 2022

elliotcmorris commented Dec 1, 2022

elliotcmorris commented Dec 1, 2022

foundrytom commented Dec 1, 2022

elliotcmorris commented Dec 1, 2022

foundrytom left a comment

feltech left a comment

JeanChristopheMorinPerso Dec 4, 2022

elliotcmorris Dec 5, 2022

[Build] Generate traits rather than hard-code them #15

[Build] Generate traits rather than hard-code them #15

Conversation

elliotcmorris commented Nov 25, 2022 • edited Loading

Testing

foundrytom commented Nov 25, 2022 • edited Loading

foundrytom left a comment

Choose a reason for hiding this comment

feltech left a comment

Choose a reason for hiding this comment

foundrytom left a comment

Choose a reason for hiding this comment

foundrytom commented Nov 30, 2022 • edited Loading

foundrytom commented Dec 1, 2022

elliotcmorris commented Dec 1, 2022

elliotcmorris commented Dec 1, 2022

foundrytom commented Dec 1, 2022

elliotcmorris commented Dec 1, 2022

foundrytom left a comment

Choose a reason for hiding this comment

feltech left a comment

Choose a reason for hiding this comment

JeanChristopheMorinPerso Dec 4, 2022

Choose a reason for hiding this comment

elliotcmorris Dec 5, 2022

Choose a reason for hiding this comment

elliotcmorris commented Nov 25, 2022 •

edited

Loading

foundrytom commented Nov 25, 2022 •

edited

Loading

foundrytom commented Nov 30, 2022 •

edited

Loading