-
Notifications
You must be signed in to change notification settings - Fork 186
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
fix #8842: add jetstream/analysis schemas #8847
Conversation
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.
Whoaaaa neeeaaattt it's beautiful 🥹
Okay so we should add some stuff like formatting/linting/tests etc, do you want to do some/any of that in this pr or just get this landed and hook more of that stuff up later?
Also I guess we should setup infrastructure to publish this to pypi and then consume it in experimenter/jetstream? That we can definitely do as followups.
But this is exactly what I was hoping for it looks great 🎉 🎉 🎉
Yea, so some of that I was considering for future PRs (publish to pypi, consumption in jetstream/experimenter). For formatting/linting, I guess I need to ensure that our existing stuff runs against this new dir, and that makes sense to setup here/now. For tests, do you want to see some tests specific to the schemas themselves, or would it be better to wait until we're using the schemas and then write tests of the functionality they enable alongside that? |
I think a minimal test of just having an example json that stresses the whole schema and running them through it would be nice here. But since this is living at the top level you'll probably need to setup something around it to get pytest going, maybe just a poetry project? Maybe wrap it in a Docker container to ensure there's a stable Python environment for it? (which is what Docker was actually invented for, funnily enough) |
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.
There are some attributes that looking in Jetstream are set to Optional
, so I think they should be optional here too.
Other than that, looks great.
schemas/mozilla_nimbus_schemas/tests/jetstream/test_analysis_errors.py
Outdated
Show resolved
Hide resolved
schemas/mozilla_nimbus_schemas/tests/jetstream/test_metadata.py
Outdated
Show resolved
Hide resolved
Makefile
Outdated
schemas_black_check: | ||
(cd schemas && poetry run black --check --diff .) | ||
|
||
schemas_ruff_check: | ||
(cd schemas && poetry run ruff .) | ||
|
||
schemas_check: schemas_black_check schemas_ruff_check | ||
(cd schemas && poetry run pytest) | ||
|
||
schemas_code_format: | ||
(cd schemas && poetry run black . && poetry run ruff --fix .) |
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.
Tried this on my local and got:
E ModuleNotFoundError: No module named 'mozilla_nimbus_schemas'
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.
Pretty sure I fixed this in the latest commit, but test this out again if you get a chance.
assert ae.metric == "test-metric" | ||
assert ae.analysis_basis == "enrollments" | ||
ae_json = ae.json() | ||
print(ae_json) |
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.
Nope, good catch. Thanks!
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.
I think the majority of the tests here aren't really needed. Pydantic has great coverage for most of what you are testing. I think the json parts of the tests would be good to keep, as you did write a custom json_loads
function.
Yea, makes sense to me. I wrote some of them partly to test how it worked, but you're right! I'll take a sweep and see what I can remove. |
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.
Reviewed again and everything looks great 🎉
So followup are we gonna publish this to pypi? 😱
Because
This commit