-
Notifications
You must be signed in to change notification settings - Fork 96
Features client #1073
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
Features client #1073
Conversation
I added some minor suggestions for the type errors. Just remembered your request for somebody picking this up so no need to follow up. Looks like the biggest piece remaining is some test coverage. Maybe one of us can tackle that during one of the working session time blocks. |
Thanks for taking a look. Test coverage is definitely a big missing piece. CLI integration would be the other major part that's lacking, since this realy only covers the SDK part right now. There's also a number of other Features API endpoints to implement - so far this covers all the major ones, but not any of the less-used ones (e.g. "alternates" or "validate") |
c7623aa
to
005dbdd
Compare
I'm spending some time finishing this up, current progress:
I'm also proposing to add a new class Feature, which is a subclass of Another proposal is that add_features accepts geojson Features, Geometries as well as objects that implement |
Added CLI commands with tests |
I did a little bit of testing of the CLI here (mostly because I happened to need to list some features today), and have a little bit of feedback on the user experience:
|
Thanks @tbarsballe! I had the same issue as you- I regularly typed For your 3rd point, is the use case to list collections, then pick a collection and list those features? |
Yeah,
Yes, that's exactly the use case. I think a --compact option like you describe would most cleanly solve it. |
Here are some options for the CLI: For listing items, we could go with either of the following (both are equivalent):
For creating an item/feature, I think we would stick with item-add unless you have another idea:
Alternately we could use another layer of sub-commands to represent collections and items/features:
Something like this is also possible, but may make it harder to present the CLI in terms of logical groups in the docs:
I'm not sure about an extra layer sub-commands myself, I personally would rather have one flat list of 6-7 commands in |
I like your first option. And yes, sticking I had also considered another layer of commands, but it just seems like added complexity for not much gain. |
Added Output of
|
Also, I figure we should probably update the SDK methods to say |
I tried out the compact mode - looks good, much more legible. |
…es, create_features
234be19
to
6d37884
Compare
I split everything out into I wasn't able to get the flat command list
|
@tbarsballe @asonnenschein @ischneider anyone able to take another look? I believe all comments should now be addressed but if I missed one let me know. |
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.
Pulled it down for a test today and ran into a new issue - if you're using an expired API key, the features CLI commands will dump a large stacktrace on you:
(.venv) torben.barsballe@torben planet-client-python % planet features collections list
Traceback (most recent call last):
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/http.py", line 296, in _raise_for_status
response.raise_for_status()
~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/httpx/_models.py", line 829, in raise_for_status
raise HTTPStatusError(message, request=request, response=self)
httpx.HTTPStatusError: Client error '401 Unauthorized' for url 'https://api.planet.com/features/v1/ogc/my/collections'
For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/bin/planet", line 8, in <module>
sys.exit(main())
~~~~^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/click/core.py", line 1157, in __call__
return self.main(*args, **kwargs)
~~~~~~~~~^^^^^^^^^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/click/core.py", line 1078, in main
rv = self.invoke(ctx)
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/click/core.py", line 1688, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/click/core.py", line 1688, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/click/core.py", line 1688, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/click/core.py", line 1434, in invoke
return ctx.invoke(self.callback, **ctx.params)
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/click/core.py", line 783, in invoke
return __callback(*args, **kwargs)
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/cli/cmds.py", line 78, in wrapper
return cmd(*args, **kwargs)
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/cli/cmds.py", line 102, in wrapper
return asyncio.run(func(*args, **kwargs))
~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/runners.py", line 194, in run
return runner.run(main)
~~~~~~~~~~^^^^^^
File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/runners.py", line 118, in run
return self._loop.run_until_complete(task)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/base_events.py", line 721, in run_until_complete
return future.result()
~~~~~~~~~~~~~^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/cli/features.py", line 82, in collections_list
output = [c async for c in results]
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/clients/features.py", line 100, in list_collections
response = await self._session.request(method='GET', url=url)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/http.py", line 404, in request
http_response = await self._retry(self._send, request, stream=False)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/http.py", line 347, in _retry
raise e
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/http.py", line 332, in _retry
resp = await func(*a, **kw)
^^^^^^^^^^^^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/http.py", line 410, in _send
http_resp = await self._client.send(request, stream=stream)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/httpx/_client.py", line 1629, in send
response = await self._send_handling_auth(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...<4 lines>...
)
^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/httpx/_client.py", line 1657, in _send_handling_auth
response = await self._send_handling_redirects(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...<3 lines>...
)
^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/httpx/_client.py", line 1715, in _send_handling_redirects
raise exc
File "/Users/torben.barsballe/repos/Planet/planet-client-python/.venv/lib/python3.13/site-packages/httpx/_client.py", line 1697, in _send_handling_redirects
await hook(response)
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/http.py", line 299, in _raise_for_status
cls._convert_and_raise(e)
~~~~~~~~~~~~~~~~~~~~~~^^^
File "/Users/torben.barsballe/repos/Planet/planet-client-python/planet/http.py", line 98, in _convert_and_raise
raise error_type(response.text)
planet.exceptions.InvalidAPIKey: {"detail":"Invalid username/password."}
Other CLI subcommands (orders, subscriptions, etc.) don't have this issue, so I assume there's some error handing missing from the features CLI part.
Aside from that, everything seems to be working fine.
I also skimmed over the last batch of changes - looks good to me, though I did add a minor comment on the doc content.
@@ -458,6 +458,59 @@ async def download_and_validate(): | |||
cl.validate_checksum(asset, path) | |||
``` | |||
|
|||
### Features API Collections and Features | |||
|
|||
The Python SDK now supports Features API Collections and Features (note: in the SDK and API, Features are often referred to as items in a collection). |
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.
Should we add a link to the platform docs on Features API?
|
||
#### Creating a collection | ||
|
||
You can use the Python SDK to create Features API collections. |
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: "You can use the Python SDK to create feature collections in the Features API."
from planet.cli.options import pretty | ||
|
||
|
||
def command(group: click.Group, |
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.
IMO this is a clever solution to the 'too many decorators' problem, however this shouldn't block shipping the Features API client. This can be it's own task that ships as a fast follow enhancement after we ship the Features API client.
|
||
Example: | ||
|
||
planet features items add my-collection-123 ./my_geom.geojson |
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.
Is there support, now or planned, to give the GeoJSON here as a JSON string?
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.
do you mean like the following:
planet features items add my-collection-123 '{"type": "Feature, "geometry": {...}}'
That's a good idea and I hadn't thought about that. If we get feedback that a user would like to do this, we could try to test whether the argument is a filename or valid json. I would probably wait to see if it's a common thing though.
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.
Yup, your example is exactly what I am thinking about. That way a user could simply copy and paste a GeoJSON geometry instead of having to put it in a file.
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.
LGTM! I left a few comments, and I see there are some other open comments, but nothing that I think should block this MR. The scope of this MR is already pretty huge, I am happy to see this merged as is and some of the lingering feedback can be fast follows.
Includes: list_collections, create_collection, list_features, create_features
This came out of an internal need for a project I was working on incorporating the SDK, and represents about the extent of time I have to contribute to Features API support at this time. If someone else can pick up from where this started, that would be great.
I have programmatically and manually tested all of the the FeaturesClient methods in the context of the project described above, and everything seems to be working. In particular, the API has different pagination scheme, currently handled by overriding Paged.__next_link, that could perhaps be incorporated directly into Paged, since the as-of-yet-unimplemented Analytics API uses the same pagination scheme.
Related Issue(s):
Oddly enough, we do not seem to have an issue for Features API support yet.
Proposed Changes:
For inclusion in changelog (if applicable):
PR Checklist: