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

Documenting "inspect" and context awareness udf.rst #617

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

daviddkovacs
Copy link

Hi, I made a quick description on how to use the "inspect" function to print within the UDFs.

moreover, I tried to give a brief explanation on how users can provide their own local side variables to be used within the UDF.

Hi, I made a quick description on how to use the "inspect" function to print within the UDFs.

moreover, I tried to give a brief explanation on how users can provide their own local side variables to be used within the UDF.
@jdries
Copy link
Collaborator

jdries commented Sep 13, 2024

Nice, thanks!
For the bit about inspecting variables, we may want to integrate it with the existing explanation about logging?
https://open-eo.github.io/openeo-python-client/udf.html#logging-from-a-udf
Or if somehow the existing explanation was hard to find, suggestions for improvement are welcome.

@soxofaan
Copy link
Member

Hi, thanks for taking the time to contribute!

About the part on inspect in UDF, I agree that it might be better to integrate that with the existing inspect docs at https://open-eo.github.io/openeo-python-client/udf.html#logging-from-a-udf or make them easier to discover (e.g. move it up or sprinkle some references around).

About the part on passing through the context: that is indeed a valid topic to document better. Also see the discussion at #520
A thing that should be added to the current PR is how to pass the context to the "parent" process of the UDF

docs/udf.rst Show resolved Hide resolved
docs/udf.rst Outdated Show resolved Hide resolved
docs/udf.rst Outdated Show resolved Hide resolved
@daviddkovacs
Copy link
Author

Hi, thanks for taking the time to contribute!

About the part on inspect in UDF, I agree that it might be better to integrate that with the existing inspect docs at https://open-eo.github.io/openeo-python-client/udf.html#logging-from-a-udf or make them easier to discover (e.g. move it up or sprinkle some references around).

About the part on passing through the context: that is indeed a valid topic to document better. Also see the discussion at #520 A thing that should be added to the current PR is how to pass the context to the "parent" process of the UDF

Yes, indeed it is better to integrate into the existing docs.
I am looking into passing the context ot the parent process of the UDF (i.e. the part NOT in apply_datacube function right ?), however I didnt manage to do that, nor found anything on it. Can you indicate me where it is shown, and I'll integrate it in the PR

@soxofaan
Copy link
Member

good point, it's not easy to find succint examples on properly using context in UDF.

We have some unit test coverage in the VITO backend on this for example at https://github.com/Open-EO/openeo-geopyspark-driver/blob/cdd731ce6d684eba894beff7c8ac78266ddf12b0/tests/test_api_result.py#L718-L889, but that's probably a bit cryptic.

I think there are two use cases to document:

  1. directly passing context to run_udf
udf = openeo.UDF(
    "...", 
    context={"factor": 12.34},
)
cube = cube.apply(udf)
  1. passing context to parent process, and pass it through in run_udf
udf = openeo.UDF(
    "...", 
    context={"from_parameter": "context"},
)
cube = cube.apply(udf, context={"factor": 12.34})

Both of these patterns have their usefulness .The first is simpler to reason about. The second is the approach to take when the context comes from "higher up", e.g. UDP parameters

@soxofaan
Copy link
Member

That being said, I think the python client should make it simpler to get that second usage pattern right. I made a ticket for that:

daviddkovacs and others added 2 commits September 13, 2024 11:46
Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com>
Copy link
Author

@daviddkovacs daviddkovacs left a comment

Choose a reason for hiding this comment

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

Added proper description, shorter lines and passing "context" to parent udf

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some more notes

docs/udf.rst Show resolved Hide resolved
docs/udf.rst Show resolved Hide resolved
========================================

In order to pass variables and values that are used throughout the user side of script, these need to be put in the `context` dictionary.
Once, these variables are defined within `context` dictionary, the UDF needs to be made context aware, by adding `context={"from_parameter": "context"}` at the end of your UDF.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Once, these variables are defined within `context` dictionary, the UDF needs to be made context aware, by adding `context={"from_parameter": "context"}` at the end of your UDF.
Once these variables are defined within `context` dictionary, the UDF needs to be made context aware, by adding `context={"from_parameter": "context"}` at the end of your UDF.

variables are defined within context dictionary

This is a bit confusing, because in your example you define them in a user_variable dictionary

Copy link
Author

Choose a reason for hiding this comment

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

true, how should I define it?

docs/udf.rst Outdated Show resolved Hide resolved
docs/udf.rst Show resolved Hide resolved
Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com>
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