Support for the federation extension in Jupyter visual outputs #771
Conversation
m-mohr
force-pushed
the
jupyter-federation-support
branch
2 times, most recently
from
670b5a1 to
4d9d376
Compare
m-mohr
force-pushed
the
jupyter-federation-support
branch
from
4d9d376 to
348a4b6
Compare
There was a problem hiding this comment.
some quick notes from initial review.
Also, if you are familiar with that: consider setting up pre-commit hooks in your local git checkout to automatically normalize code formatting
see https://open-eo.github.io/openeo-python-client/development.html#pre-commit-for-basic-code-quality-checks for instructions
| @@ -626,7 +627,8 @@ def extent(self) -> dict: | |||
| return self._orig_metadata.get("extent") | |||
|
|
|||
| def _repr_html_(self): | |||
| return render_component("collection", data=self._orig_metadata) | |||
| federation = self._connection.capabilities().ext_federation_backend_details() | |||
There was a problem hiding this comment.
this will fail with the self._connection's default value None
| @@ -505,8 +505,9 @@ class CollectionMetadata(CubeMetadata): | |||
|
|
|||
| """ | |||
|
|
|||
| def __init__(self, metadata: dict, dimensions: List[Dimension] = None): | |||
| def __init__(self, metadata: dict, dimensions: List[Dimension] = None, connection = None): | |||
There was a problem hiding this comment.
I understand what you do here, but this simple addition creates an architectural a problem: it couples a container of collection metadata to a client-side connection object:
- the connection object is a non-trivial class with quite a lot of dependencies, so this increases the risk on import cycle issues (which is already an existing problem).
- I was also going to give the review note to give the
collectiona type annotation, but when you do that (and you might already have tried that), you will get in import cycle/dependency hell, which proves my point
- I was also going to give the review note to give the
- We also use the
CollectionMetadataclass at the backend-side to manage cube/collection metadata, but there is no connection object of any kind in that context, so this argument addition is introducing a mismatch in expectations
I have to look deeper in the PR and think it through some more,
but at first sight, you just need the connection object to get the capabilities, to get listing of backends in the federation, to be passed to render_component. These are a lot of concerns I'd like to keep outside of this class. Wouldn't it be an option to instead just add an argument like, say _repr_html_extra : dict, which you then merge into data at _repr_html_ time.
There was a problem hiding this comment.
Another solution would be to add layers to the class hierachy:
- remove
_repr_html_ fromCollectionMetadataclass (to eliminate all client-side connection concerns) so that it can be used also in backend-side contexts - Introduce a child class, e.g.
RichCollectionMetadataorVisualCollectionMetadata, defined under theopeneo.rest"namespace", which just adds_repr_html_to provide rich Jupyter visualisation
There was a problem hiding this comment.
Would it improve things if we'd just pass over the capabilities object?
Implements the Jupyter-related part of #668: Support for the federation extension in Jupyter visual outputs.
Didn't implement any additional tests as it's all just within the components, we just pass additional data in and the existing tests still pass.
Example:
