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] processing.py updates #411

Merged
merged 10 commits into from
Apr 17, 2023

Conversation

clausmichele
Copy link
Member

Adapted the syntax to comply with the new versions of the dependencies.

@soxofaan
Copy link
Member

soxofaan commented Apr 5, 2023

is this related to the version bumps of #409?

@clausmichele
Copy link
Member Author

Yes indeed

@soxofaan
Copy link
Member

soxofaan commented Apr 6, 2023

I probably didn't make myself clear when I proposed to split up #407 :
these dependency bumps should be part of PR #411 and not be done as a separate PR (#409 in this case)

(I guess it's the same thing with #410 and the rioxarray bump of #409)

openeo/local/processing.py Outdated Show resolved Hide resolved
openeo/local/processing.py Outdated Show resolved Hide resolved
openeo/local/processing.py Outdated Show resolved Hide resolved
@clausmichele
Copy link
Member Author

clausmichele commented Apr 6, 2023

I probably didn't make myself clear when I proposed to split up #407 : these dependency bumps should be part of PR #411 and not be done as a separate PR (#409 in this case)

(I guess it's the same thing with #410 and the rioxarray bump of #409)

indeed, I will merge the two PRs

Copy link
Member

@PondiB PondiB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾

requirements.txt Outdated Show resolved Hide resolved
process_registry[func.__name__] = Process(
spec=specs[func.__name__], implementation=func
)
return process_registry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, I think this can be done more compactly with:

for name, func in inspect.getmembers(
    openeo_processes_dask.process_implementations,
    inspect.isfunction,
):
    process_registry[name] = Process(
        spec=getattr(openeo_processes.specs, name),
        implementation=func
    )

Also, wouldn't it be better that openeo_processes_dask provide helpers to list implementations/spec, instead of having to use ugly inspect.getmembers and getattr constructs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukeWeidenwalker sorry if I tag you again, but I wasn't involved in this development of openeo-processes-dask so I couldn't tell the reason.

Copy link

@LukeWeidenwalker LukeWeidenwalker Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah apologies, I missed this tag - yes, agree with Stefaan, this is something openeo-processes-dask could do better - have created an issue to do this at some point!
Open-EO/openeo-processes-dask#92

openeo/local/processing.py Outdated Show resolved Hide resolved
@clausmichele
Copy link
Member Author

@soxofaan please merge this when you have time, so that we can include it in a release before SRR5

@soxofaan soxofaan merged commit 5dd398c into Open-EO:master Apr 17, 2023
@soxofaan
Copy link
Member

merged in master

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.

4 participants