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

Pydantification #155

Merged
merged 7 commits into from
Feb 11, 2025
Merged

Pydantification #155

merged 7 commits into from
Feb 11, 2025

Conversation

JGoldstone
Copy link
Contributor

This PR converts camdkit to use Pydantic. As compared to the previous PR attempting to do the same:

  • it is on a new branch off of main, and can be automatically merged.
  • references to developer-system-local regression references have been converted to references inside the source tree; in particular, copies of the pre-Pydantic schema and subschemas and examples are now in ...test/resources/.

@jamesmosys
Copy link
Collaborator

I've looked through this and I think the new approach is excellent. @JGoldstone can you confirm that the outputs - i.e. build/schema.json and the build/examples/* are identical to the published ones?

@JGoldstone
Copy link
Contributor Author

Will check the match one more time. Almost 100% sure the examples match; will recheck schema, but I don't think I would have said it's ready without that matching as well.

@JGoldstone
Copy link
Contributor Author

The example regressions were running but because of a missing main() in test_schema_regression.py, that coefficient wasn't running. Are you OK with these differences? If something isn't self-evident I can go over the rationale item-by-item:
image

@JGoldstone
Copy link
Contributor Author

To go through those verbally (and they all have to do with the new-ish PTP stuff):

  • Pydantic writes a real number having an inclusive lower bound of zero as 0.0 as opposed to the hand-coded 0
  • The description was missing in the hand-coded schema for PTP synchronization priorities so I added it
  • There were no bounds on the leader identity string: it could be zero-length, it could be 4 GB
  • There was no upper bound on the vlan integer. I was able to find instances on the web where it was greater than 65535, so I made it have a max of an unsigned 32-bit int
  • There was no validity check on the values of the time source string, so I looked into the code and found the three values we define and limited acceptable values to one of "GNSS", "Atomic clock" and "NTP"

@JGoldstone
Copy link
Contributor Author

JGoldstone commented Feb 10, 2025

If you're good with those, I will hand-update the reference schema json against which the generated schema is compared to be as shown above, then check in that and the fix to add main() etc to test_regression_schema.py. And push that change.

Copy link
Collaborator

@jamesmosys jamesmosys left a comment

Choose a reason for hiding this comment

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

The minor schema changes LGTM.

@jamesmosys jamesmosys merged commit 0a4372e into SMPTE:main Feb 11, 2025
1 check failed
@JGoldstone JGoldstone deleted the pydantification branch February 11, 2025 21:54
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.

2 participants