feature: artifact-helper - #750
Conversation
soxofaan
left a comment
There was a problem hiding this comment.
this is quite a large diff 😄 and I haven't gone deep yet, but here are some initial notes
soxofaan
left a comment
There was a problem hiding this comment.
I'm a bit short on resources to go deep, so just some superficial notes:
This PR adds a lot of new things (lot of new modules, lot of classes) to the public API of the openeo package. Is it necessary to make all of this public? Once something is public, it's hard to change things because you have to care about compatibility. It's also overwhelming to the user I think.
I also think this feature should be documented in the docs, e.g. somewhere under https://open-eo.github.io/openeo-python-client/cookbook/index.html . Doing that documentation exercise will also make it more clear what the minimal public user-facing part of the API is.
I see you already did some docs on ArtifactHelper, so it should be easy to bootstrap initial docs from that like done at
openeo-python-client/docs/cookbook/job_manager.rst
Lines 13 to 14 in 7e65087
d78de33 to
f6b3809
Compare
|
@soxofaan provided docs as requested. I did move around code but mostly renames of files to start with _ to discourage users from entering. Other changes:
|
d3d610d to
2dd009f
Compare
|
Unittests are failing because of another change: #788 |
46c5b80 to
020627f
Compare
soxofaan
left a comment
There was a problem hiding this comment.
trying to squeeze in some more review notes before the holidays
| } | ||
|
|
||
|
|
||
| class ArtifactHelper(ArtifactHelperBuilderABC): |
There was a problem hiding this comment.
To be honest I'm getting a bit confused by these helper and helperbuilder class hierarchies.
there is ArtifactHelperBuilderABC and ArtifactHelperABC in _artifact_helper_abc.py
And here you have ArtifactHelper which is subclass of ArtifactHelperBuilderABC (instead of ArtifactHelperABC). Or should this call be called ArtifactHelperBuilder?
There was a problem hiding this comment.
While I understand the need for a hierarchy of helper classes,
I'm not sure I understand the need for a hierarchy for helperbuilder classes.
Wouldn't you just need a single helper builder endpoint (e.g. a single function) instead of a whole hierarchy?
There was a problem hiding this comment.
I think you are right. I believe this stems from when it was unclear to use the capabilities and to allow multiple builder implementations but this is adding unnecessary complexity early on. As you pointed out in he first comment the name is also not the greatest adding confusion into the mix so a 1-function builder endpoint sounds much more appropriate. I replaced the logic with a build_artifact_helper as build_ is used already in other places of the code base.
Inspired by and test for PR #750
|
I would also add a changelog entry along the lines of |
867d403 to
de1afc2
Compare
soxofaan
left a comment
There was a problem hiding this comment.
I'd merge latest master in this feature branch
Allow uploading artifacts using: ``` from openeo.extra.artifacts import build_artifact_helper artifact_helper = build_artifact_helper(connection) storage_uri = artifact_helper.upload_file(path, object_name) presigned_uri = artifact_helper.get_presigned_url(storage_uri) ```
0802667 to
d9c6eb2
Compare
Inspired by and test for PR #750
Inspired by and test for PR #750
Allow uploading artifacts using:
Where object_name is a logical string name that will be used to give a unique name in the backend storage.
And src_file_path points to a local file that needs to be stored remotely.
This MVP version still has the sts and s3 URIs hard coded. This is temporary. It is possible to expand the links that are exposed at the root_url (https://openeo.dataspace.copernicus.eu/openeo/1.2/) to include rels like 'artifacts-s3-endpoint' and 'artifacts-sts-endpoint'. That way different artifact backends can be chosen based on the OpenEO environment.