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

save_result (and save_model): Is the boolean return value useful at all? #334

Closed
m-mohr opened this issue Mar 9, 2022 · 1 comment · Fixed by #401
Closed

save_result (and save_model): Is the boolean return value useful at all? #334

m-mohr opened this issue Mar 9, 2022 · 1 comment · Fixed by #401
Labels
question Further information is requested
Milestone

Comments

@m-mohr
Copy link
Member

m-mohr commented Mar 9, 2022

Process ID: save_result (and save_model)

Describe the issue:

@soxofaan has a good point in #304 (comment) :

if the storage failed, an error should be thrown, it's just too error prone to return a boolean nobody is going to check.

save_result returns false in case of some failures, but throws in other cases (FormatUnsuitable for example). I doubt that in case of a failures, any implementation will actually return false. Instead they likely all throw an error and fail. Thus, the only value you may effectively get from this process is "true".
Also, it's actually not well defined what would fail and what would result in false. So this is an interoperability issue.

The only reason for a boolean I could find it that if you have two save_result nodes that then you can save partial results, but that seems like a strange thing if other issues throw anyway.

Proposed solution:

His proposal:

I think it's cleaner to return nothing (null) or return the original data

Context: There's no void in openEO, null is a strange replacement. Effectively, then we could also clarify that it's always true and throws otherwise. Changing to null doesn't bring any real benefit here. So we could return the original data instead. I actually think this was already proposed somewhere else, too. On the other hand, you can simply connect any following processes after save_result with the previous node and get the same behavior. Should we clarify that save_result always throws and otherwise always returns true? Or should we completely redefine the return value?

Additional context:
Should go through PSC if a breaking change is proposed.

@m-mohr m-mohr added bug patch PSC vote question Further information is requested and removed bug patch labels Mar 9, 2022
@m-mohr m-mohr linked a pull request Jan 31, 2023 that will close this issue
@m-mohr m-mohr added this to the 2.0.0 milestone Feb 1, 2023
@m-mohr m-mohr removed the PSC vote label Mar 14, 2023
@m-mohr m-mohr closed this as completed Mar 14, 2023
@m-mohr
Copy link
Member Author

m-mohr commented Mar 14, 2023

We discussed this today and keep the boolean result. All options where chaining is required can be solved otherwise and may just be back-end limitation. This way we keep the option to later use the return value for something more useful, e.g. to return a vector data cube for each generated STAC item.

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

Successfully merging a pull request may close this issue.

1 participant