-
Notifications
You must be signed in to change notification settings - Fork 194
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
feat(nimbus): V7 schema #12170
base: main
Are you sure you want to change the base?
feat(nimbus): V7 schema #12170
Conversation
for some reason command |
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 don't think we should add a whole 'nother module for this. Instead, can we define v7 api mixins and make a class that inherits from the v6 api schemas and the mixins?
schemas/mozilla_nimbus_schemas/experiments_v7/experiments_v7.py
Outdated
Show resolved
Hide resolved
schemas/mozilla_nimbus_schemas/experiments_v7/experiments_v7.py
Outdated
Show resolved
Hide resolved
yeah @mikewilli and I were discussing this too, for the first approach we kept out separately |
I don't feel strongly either way, but it seems like there's a lot of duplication here so @brennie's suggestion makes sense to me. |
thank you both @mikewilli @brennie , I was thinking of keeping v7 separately but reusing the v6 code, does it seem to be the right approach? |
Yeah its fine to keep the v7 api separate. Maybe we rename the module experimenter_apis or something? And then rename the top level object the experimenter_apis.ExperimentV7 ? |
026481e
to
d88cf61
Compare
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.
Looking good, left some suggestions and especially want to make sure that the interface exposes the new sub-packages.
schemas/mozilla_nimbus_schemas/experimenter_apis/experiments/experiments.py
Outdated
Show resolved
Hide resolved
schemas/mozilla_nimbus_schemas/experimenter_apis/experiments_v7/experiments_v7.py
Outdated
Show resolved
Hide resolved
schemas/mozilla_nimbus_schemas/tests/experiments_v7/test_experiments_v7.py
Outdated
Show resolved
Hide resolved
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.
Not worth blocking approval, but did you consider the V7-at-the-end naming?
Otherwise LGTM!
done, v7 at the end now |
Thanks everyone for hopping on this, I think it's all looking good, but I'm confused about one thing. I see that there's a folder for v7 fixtures that includes desktop recipe examples, but v7 isn't used in the client and doesn't need to maintain any client compatibility or backwards compatibility, so I'm confused about why that's there? |
I wasn't sure about the scope of the v7 usage, just added those in case. do you think I should take it out @jaredlockhart? |
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.
Since V7 isn't consumed by clients
- We don't need special cases for desktop/sdk
- We don't need to test against client recipes
- Let's have a sample fixture for v7 as well, but it's actually separate from the existing client fixtures, just generate a factory locally, run it through v7, snapshot that.
And let's keep naming consistent so if we're going to identify things v7 we should also identify things as v6.
I'm against renaming the v6 to have "V6" since they're the canonical representations (and also means we'd have to update our clients to look for new schemas) I like the idea of separating the schemas into experiments schemas and API schemas (i.e., |
@jaredlockhart @freshstrangemusic I removed the client from v7, I haven't changed the old one to v6 as Beth mentioned that clients need to make changes too, let me know if its necessary I am happy to rename that too v6 |
model_config = ConfigDict(use_enum_values=True) | ||
|
||
|
||
class NimbusExperimentV7(BaseExperimentV7): |
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.
Splitting tihs up into multiple classes is unnecessary. Please join these two into a NimbusExperimentV7
model_config = ConfigDict(title="ExperimentLocalizations") | ||
|
||
|
||
class _CommonBaseExperimentBranch(BaseModel): |
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.
This class should not have a leading underscore. Please rename it back to BaseExperimentBranch
) | ||
|
||
|
||
class _CommonDesktopExperimentBranch(_CommonBaseExperimentBranch): |
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.
This class is unnecessary. Please move this back into DesktopExperimentBranch (it only has one subclass).
) | ||
|
||
|
||
class _CommonSdkExperimentBranch(_CommonBaseExperimentBranch): |
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.
This class is unnecesssary. Please move this back into SdkExperimentBranch as it only has one subclass.
class BaseExperimentV7(BaseModel): | ||
"""The base experiment definition for V7.""" | ||
|
||
schemaVersion: str = Field(description="Version of the NimbusExperiment schema.") |
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.
We previously discussed moving all the comomn fields between the v6 and v7 api into a separate model inheriting from that. What happened?
Because
This commit
Fixes #12060