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

Avoid setting default values for process arguments #500

Open
soxofaan opened this issue Nov 9, 2023 · 3 comments
Open

Avoid setting default values for process arguments #500

soxofaan opened this issue Nov 9, 2023 · 3 comments

Comments

@soxofaan
Copy link
Member

soxofaan commented Nov 9, 2023

from Open-EO/openeo-processes-dask#103 (comment)

In case of resample_spatial however, we do always add the align parameter with default value "upper-left"

I think this should be changed. It should only be sent if not the default value and supported by the back-end.

A lot of DataCube methods adapted the default argument values from openeo-processes specs.
For example cube.resample_spatial() (without explicit align arg) will silently set align="upper-left" in the resulting process graph.

This has some subtle effects:

  • if a back-end chooses a different default value than the "official" specs, this will not be picked up automatically unless the user manages to unset the argument in the process graph in some way (which in the resample_spatial example above would be pretty ugly to do)
  • if a backend chooses not to support an argument, it will still receive it from the python client, which might break back-end side validation. Again, it is pretty ugly to workaround this.

So, it should be a general rule on DataCube methods to only adapt explicitly specified method arguments as process arguments in the process graph.

@soxofaan
Copy link
Member Author

soxofaan commented Nov 9, 2023

A counter argument to consider: the current approach allows the python docs to list the default values in a standardized way as method argument defaults. If this is dropped, it will be more work for the end user to discover the actual default values.

@jdries
Copy link
Collaborator

jdries commented Nov 9, 2023

Note: I'm also not buying the original reasoning that triggered this. If a backend only supports one of the values in an enum based argument, it makes sense for me to list that single value rather than drop the entire argument.

@clausmichele
Copy link
Member

@christophreimer @ValentinaHutter @GeraldIr @SerRichard please check this, otherwise we might encounter more issues if you expose processes without some parameters.

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

No branches or pull requests

3 participants