-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[Python] Add a symlink to the OpenAPI rest catalog spec and package it with the python library #4545
Conversation
@@ -45,6 +45,8 @@ python_requires = >=3.7 | |||
install_requires = | |||
mmh3 | |||
singledispatch | |||
pyyaml |
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.
Does this make pyyaml a dependency of the library itself? If so, is there anyway to make it just a “test” dependency or something or will we need it at runtime?
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'll need it at runtime. Specifically in the TableMetadata
class which hasn't been merged in yet but the idea is that reading in a table metadata file will be validated against the open api spec.
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.
Are we gonna do that for every file on every read? Or just as a test?
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's a great question and I think we can decide on that later. The TableMetadata PR is still a WIP but that class includes this validate method. So we may have certain paths where we don't validate (loading a metadata file from a path we got from a catalog) and other paths where we do validate (like before writing+committing a new metadata file), just as an example.
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 it would be good to pin it on a version. Currently it will just install the latest greatest, but there might be breaking changes (happens all the time in Python land :)
pyyaml | |
pyyaml==6.0 |
|
||
@lru_cache(maxsize=1) | ||
def read_yaml_file(path: str): | ||
return yaml.safe_load(open(path)) |
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: If something happens, we'll clean up the connection:
with open(path) as f:
return yaml.safe_load(f
|
||
|
||
@lru_cache(maxsize=1) | ||
def read_yaml_file(path: 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.
def read_yaml_file(path: str): | |
def read_yaml_file(path: str) -> Dict[str, Any]: |
return yaml.safe_load(open(path)) | ||
|
||
|
||
def load_openapi_spec(): |
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.
def load_openapi_spec(): | |
def load_openapi_spec() -> Dict[str, Any]: |
Co-authored-by: Fokko Driesprong <fokko@apache.org>
Closed this since there's still some discussions around how we want to add schema validation and that can be postponed without blocking anything. |
This adds a symlink for the REST catalog OpenAPI spec contained in
open-api/rest-catalog-open-api.yaml
and a utility function to load it. The function uses@lru_cache
to cache the file after it's loaded once.