[WIP] Extensible tag classification model discovery through Entry Points#463
[WIP] Extensible tag classification model discovery through Entry Points#463Roel Bollens (RoelBollens-TomTom) wants to merge 4 commits intodevfrom
Conversation
Roel Bollens (RoelBollens-TomTom)
added
the
change type - cosmetic 🌹
Roel Bollens (RoelBollens-TomTom)
force-pushed
the
discovery-rework-with-tags
branch
from
b8ca2fe to
c00022c
Compare
| if namespace and namespace != "overture": | ||
| filtered_models = { | ||
| key: model_class | ||
| for key, model_class in all_models.items() | ||
| if namespace in key.tags | ||
| } | ||
|
|
There was a problem hiding this comment.
I doubt that anyone's relying on --namespace right now, so I'd drop this in favor of the --tag <namespace> support that you've added.
There was a problem hiding this comment.
I had left the json-schema and validate commands to kept their existing interface and modified them to maintain backward compatibility, I still plan update then (and codegen cli's commands) but left it a mystery as if it would be done as part of this PR or an other one.
There was a problem hiding this comment.
Makes sense. My point was to not worry about backward-compatibility.
There was a problem hiding this comment.
To be fair, I didn't care about making it backwards compatible, it was just the shorter pain. But I'll refactor the CLI commands in this PR now that codegen cli is also in, that will also resolve some of the other issues.
| models = discover_models(namespace=namespace) | ||
| actual_themes = {key.theme for key in models.keys()} | ||
| models = discover_models() | ||
| actual_themes = {next(iter(tags_by_key(key.tags, "overture:theme")),None) for key in models.keys()} |
There was a problem hiding this comment.
Extract next(iter(tags_by_key(key.tags, "overture:theme")), None) into a helper?
Ah, but where (since theme isn't a system concept)? Maybe core, but the intent is for codegen to drop its dependency on core in favor of system after this PR.
| ) -> None: | ||
| """Test that resolve_types returns models from expected themes.""" | ||
| from overture.schema.core.discovery import discover_models | ||
| from overture.schema.system.discovery import discover_models |
There was a problem hiding this comment.
My bad from before; we should aim to eliminate non-top-level imports unless they're absolutely necessary. I'd like to add a linting rule to catch this at some point (my local linter config adds this now, so codegen should be clean).
| return { | ||
| tag | ||
| for tag in tags | ||
| if TAG_RE.match(tag) and not tag.startswith(reserved_namespaces) |
There was a problem hiding this comment.
_filter_tags builds prefix checks from RESERVED_TAGS, so "overture" here implicitly reserves both the plain tag and the overture: prefix. What happens when you need to reserve a plain tag that shouldn't also claim a prefix namespace?
There was a problem hiding this comment.
As above.
packages/overture-schema-core/src/overture/schema/core/tag_providers.py
Outdated
Show resolved
Hide resolved
| filters = [] | ||
|
|
||
| if tags: | ||
| filters.append(lambda key: all(tag in key.tags for tag in tags)) |
There was a problem hiding this comment.
all() here means --tag foo --tag bar requires BOTH tags (AND). Is that the intended semantics, or should repeated --tag flags use OR?
There was a problem hiding this comment.
This was intentional, but I guess this should still be formalized so I'll leave this open for discussion.
from the coding session minutes:
--tag semantics are AND
The mockups assume--tag overture --tag system:featuremeans "must have both tags" (AND). This contradicts the design doc's stated OR semantics. The group implicitly operated with AND throughout the exercise and no one objected. This should be formalized.
There was a problem hiding this comment.
Ah, great. I'm good with that; it didn't track with how I'd been thinking about it earlier is all.
There was a problem hiding this comment.
I'll still leave this open. Maybe after refactoring the other CLI commands opinions might change.
| } | ||
| TAG = r"[a-z0-9][a-z0-9_-]*" | ||
| NAMESPACE_TAG = r"[a-z0-9]+:[a-z0-9]+(?:=[a-z0-9_.-]+)?" | ||
| TAG_RE = re.compile(rf"^(?:{TAG}|{NAMESPACE_TAG})$") |
There was a problem hiding this comment.
TAG allows hyphens and underscores ([a-z0-9_-]), but NAMESPACE_TAG's key segment doesn't — acme:my-ext would fail validation even though my-ext is a valid plain tag. Intentional?
There was a problem hiding this comment.
The tag definition/validation needs some more love. I added a regex to have some validation but very little thought went into it. Same where TAG doesn't allow for decimal points/periods were a value in a namespace tag would (3.14 would fail but acme:pi=3.14 will pass).
There was a problem hiding this comment.
That handling of numbers makes sense to me. Raw tags with decimals feels somewhat confusing, so while it's restrictive, I could justify it.
packages/overture-schema-system/src/overture/schema/system/discovery.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Seth Fitzsimmons <sethfitz@amazon.com>
Co-authored-by: Seth Fitzsimmons <sethfitz@amazon.com>
2 participants
Extensible tag classification model discovery through Entry Points
This replaces the hardcoded model classification system with tag-based classification model discovery through Entry Points. This is based on #440 by Seth and several schema (ad-hoc) coding sessions where Seth, Vic, Dana, Tristan and Roel participated in.
Model discovery moved into
system, eliminating assumptions about Overture in the process. The hardcodednamespaceconcept ("overture","annex") and theModelKindclassifier is replaced with with tags -- string labels derived by tag providers. Tags become the filtering, grouping, and classification mechanism for model discovery, driven by introspection and package metadata rather than central coordination.systemprovides generic tag-based grouping without understanding what any particular tag means. Any package can register tag providers that classify models without special support in the discovery layer.Purpose
Tags serve three roles:
(
--tag system:feature,--tag draft)mark models as vetted or approved by an authority
future extension catalog
These roles overlap -- a tag like
overture:theme=buildingsserves both filtering and taxonomy. The design accommodates this overlap through structured tags that encode both ownership and dimension.Tag Format
Tags are strings following the pattern
[prefix:]key[=value]:overture,draft,featuresystem:extension--:separates ownershipoverture:theme=buildings:signals ownership and enables prefix reservation (see Privileged Packages and Tag Reservation).=signals a dimension with a value (groupable via--group-by). One level of each -- no nested colons or multiple=signs.Minimal launch set
system:featureoverture:theme=<theme>buildings,transportation)overture:officialoverture:feature(vs.system:feature)system:feature(geometry-aware base) andoverture:feature(adds theme, type, sources, version) surfaced during the use-case exercise. Both tags exist implicitly in the inheritance hierarchy, and both should be present when applicable (an Overture feature gets both). This was deferred and likely will still drop off from the initial setExtensions
Additional extensions and accompanied tags will be introduced in a future PR. Extensions allows to augment existing types with new fields (columns).
CLI
The
list-typescommand has been updated to support filtering and grouping by tags. Currently, it no longer displays the description or fully qualified class name. Thejson-schemaandvalidatecommands kept their existing interface but have been modified to maintain backward compatibility. Further changes can be introduced in a future update.Examples
Deviations