Skip to content
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

[localprocessing] updates #407

Closed
wants to merge 1 commit into from
Closed

Conversation

clausmichele
Copy link
Member

@clausmichele clausmichele commented Mar 30, 2023

New PR to update the localprocessing functionalities:

  • Fixes dependencies
  • Code now parsing correctly band names for geoTIFFs if are present and set them in the metadata.

@soxofaan the openeo-python-client requires Python 3.6+. However, some of the functionalities I implemented require Python 3.9+. Is it possible to specify this somewhere?

I will also add the documentation to this PR later.

Code now parsing correctly band names for geoTIFFs if are present and set them in the metadata.
@clausmichele
Copy link
Member Author

@soxofaan I would like to include sample data in the project. What's the best way of doing it? I would like to include it so that a user could test the local processing without having to interact with any cloud provider at the beginning. Additionally, it could be used as a reference file for the accepted structure and what the code expects to find.

@soxofaan
Copy link
Member

Hi @clausmichele, some feedback:

I see you are working on different things in this PR. I would recommend to make separate PRs for separate fixes. That will make it easier to review, merge and move forward. If you lump multiple unrelated things together in a single PR, or worse, in a single commit, then progress is dictated by the weakest link in the PR, and things grind to a halt.

@soxofaan the openeo-python-client requires Python 3.6+. However, some of the functionalities I implemented require Python 3.9+. Is it possible to specify this somewhere?

can you give examples of where 3.9 is required? Isn't it possible to support 3.7 at least?

I would like to include sample data in the project

Can you give an idea on what volume of data we are talking here?
Usually it's a bad idea to include (large) binary files in a git repo.
If it's just one or two files of a couple of kilobytes, then it's doable.
Otherwise a solution would be to create a fresh repo to hold these binary assets

@clausmichele
Copy link
Member Author

Ok, I will split into multiple PRs.

can you give examples of where 3.9 is required? Isn't it possible to support 3.7 at least?

Python 3.9 is required by openeo-processes-dask and openeo-pg-parser-networkx . @LukeWeidenwalker could you tell us some info about this?

Otherwise a solution would be to create a fresh repo to hold these binary assets

Exactly, I also had the same idea but I wanted another opinion on this.

@LukeWeidenwalker
Copy link

Python 3.9 is required by openeo-processes-dask and openeo-pg-parser-networkx . @LukeWeidenwalker could you tell us some info about this?

Both of these projects are currently only tested against 3.9 and 3.10. When I initially set up the CI for these, I briefly tried out 3.8 and 3.11 too, but tests for those didn't pass immediately and I didn't want to spend any more time on it then.

  • I'd be open to support 3.8, although we do use some type hint syntax that's only available in 3.9, so would need some work
  • I'm not keen on supporting 3.7, because its end-of-life is coming up in 3 months: https://endoflife.date/python.

Feel free to open a PR for 3.8 support, you can just start by enabling it in CI and seeing what breaks!

@soxofaan
Copy link
Member

If python 3.9 is required just for optional dependencies but the added code in the python client repo itself is still 3.7 compatible (e.g. syntax-wise), I don't think we have to take additional measures (in the python client).
It will be a problem for users that want to use the localprocessing feature but are stuck on an older version of python (e.g. pip install will fail), but that's not something to be addressed in python client project.

I'm not keen on supporting 3.7, because its end-of-life is coming up in 3 months: https://endoflife.date/python.

I think that's fine for projects like openeo-processes-dask that are not for general consumption.
In the python client however we try to be very conservative and try to stick as long as possible to old (and even end-of-lived) python versions, because it's not always possible for users to "just upgrade" to a new version (e.g. because they have no permissions to do so).

@clausmichele
Copy link
Member Author

Good. So, the current code syntax in this project must be Python 3.6+ compatible, but the optional dependencies for localprocessing could require a newer version of Python. I will add this into the documentation I'm preparing.

By the way, @soxofaan I will close this PR and create separate ones for each component that I update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants