-
Notifications
You must be signed in to change notification settings - Fork 47
Convert voluptuous schema to msgspec #752
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
base: main
Are you sure you want to change the base?
Convert voluptuous schema to msgspec #752
Conversation
…aph into schema-conversion
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.
Thanks, this is shaping up nicely!
I don't think I have any other major concerns, it's mostly all nits and a minor changes. So I think you can go ahead and start converting Gecko without fear of a major new request (from me at least :p)
...utter.project_name}}/taskcluster/{{cookiecutter.project_slug}}_taskgraph/transforms/hello.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.
Nice, thanks for the updates! I don't see any fundamental problems or anything, though I'll reserve the right to request future changes as we test this out in Gecko ;)
I'll avoid approving this for now because I want to hold off landing until we have a working patch for Gecko, but I think you can go ahead and get started on that! Hopefully claude or some clever macros can help with the conversion, because there's going to be a lot of schemas :)
87e0de9
to
750511c
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.
I didn't get a chance to review the whole thing, but I'll leave the comments I had so far for now and resume tomorrow. Figured you might want to get a head start
from taskgraph.util.schema import Schema | ||
from taskgraph.transforms.base import TransformSequence | ||
# Define the schema using Schema base class for better type checking and performance. |
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.
nit: I'd cut out for better type checking and performance
here. The user reading this won't know what you're comparing against
|
||
# Optional fields | ||
project_regex: Optional[str] = None # Maps from "project-regex" | ||
ssh_secret_name: Optional[str] = None # Maps from "ssh-secret-name" |
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 guess this maps from works because in the base class we pass in the "rename" arg. Out of curiosity, what does it look like if we wanted to use underscores rather than dashes?
repositories: Dict[str, Repository] | ||
|
||
# Optional fields | ||
register: Optional[str] = None |
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 feel bad for not commenting this earlier, but do you think we could preserve the old descriptions as comments? The schema isn't really documented other than these descriptions, so I feel it's important that we preserve them somehow. Even if it's just a comment
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 Claude should be able to do this if you get it to examine the diff and teach it about the mapping from dash to underscore.
class RunConfig(Schema): | ||
"""Run transforms configuration.""" | ||
|
||
use_caches: Optional[Union[bool, List[str]]] = None # Maps from "use-caches" |
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 shouldn't just be List[str]
, there are only specific values allowed here (similar to TaskPriority
)
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.
A few more comments, still not all the way through though.
k: v for k, v in kebab_params.items() if k in ext_fields | ||
} | ||
if ext_params: | ||
msgspec.convert(ext_params, ext_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.
It looks like this for loop is duplicated in both branches of this if statement. So I think you can consolidate it a bit.
# List of package tasks this docker image depends on. | ||
packages: Optional[List[str]] = None | ||
# Information for indexing this build so its artifacts can be discovered. | ||
index: Optional[Any] = None |
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.
You should import the schema from gecko_taskgraph.transforms.task
and reference the index
key there when we come across these "cross references".
# Description of the task. | ||
description: str | ||
# Fetch configuration with type and additional fields. | ||
fetch: Dict[str, Any] # Must have 'type' key, other keys depend on type |
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 should use a sub schema rather than __post_init__
.
class StaticUrlFetchSchema(Schema): | ||
"""Configuration for static-url fetch type.""" | ||
|
||
type: str |
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 this should be type: Literal["static-url"]
rather than str
class GitFetchSchema(Schema): | ||
"""Configuration for git fetch type.""" | ||
|
||
type: str |
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.
type: Literal["git"]
"partner-repack-ids", | ||
"component", | ||
"build-type", | ||
] |
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 guess there's no getting around this duplication on Python 3.10 and below.. unfortunate, but not the end of the world.
# dependency. Attributes of the upstream task may be used as | ||
# substitution values in the `artifact` or `dest` values of the | ||
# `fetches` entry. | ||
fetches: Optional[Dict[str, List[Union[str, Dict[str, str]]]]] = None |
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.
Please reference the fetches schema directly if possible.
No description provided.