-
Notifications
You must be signed in to change notification settings - Fork 119
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
Sensor Agent Doc Integration Example #1180
Sensor Agent Doc Integration Example #1180
Conversation
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
…er/flytesnacks into sensor-agent-doc-v2
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
examples/sensor/README.md
Outdated
|
||
``` | ||
pyflyte run --remote \ | ||
https://raw.githubusercontent.com/flyteorg/flytesnacks/master/examples/sensor/sensor/sensor.py wf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need two sensor folder. /sensor/sensor/...
? can we update the file structure here, update it to
- sensor
| _ file_sensor_example.py
| _ http_sensor_example.py
| _ etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all the other examples are like this, and have a README and Dockerfile in the top-level directory, and an __init__.py
and example (or examples) in the enclosed directory. This PR appears to be missing a Dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Future-Outlier I think you will need to create some missing files so your example matches the directory structure shown in the contribution guide. Specifically, I think you need to add a Dockerfile, requirements.txt, and requirements.in file.
@pingsutw does that seem right to you?
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@neverett mind taking a look when you get a chance, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the information looks good, and I suggested some ways to reorganize the content so it flows a bit better. Let me know if you have any questions!
# | ||
# This example shows how to use the `FileSensor` to detect files appearing in your local or remote filesystem. | ||
# | ||
# To begin, import the required libraries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend "First" instead of "To begin"
from flytekit.sensor.file_sensor import FileSensor | ||
|
||
# %% [markdown] | ||
# Create a FileSensor task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend changing this to "Next, create a FileSensor task" and moving the next code block directly under this sentence.
# %% [markdown] | ||
# Create a FileSensor task. | ||
# | ||
# The sensor will search for the file at the specified path. If the file exists, it will return a succeed status. Otherwise, the sensor will continue running until the file is added to the directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend changing this to "To use the FileSensor created in the previous step, you must specify the path
parameter. In the sandbox, you can use the S3 path" and moving the last code block directly below it.
# | ||
# In the sandbox, you can use the s3 path. | ||
# | ||
# We have already set the minio credentials in the agent by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend moving this section the very bottom and putting it in a note, like so:
# % [markdown]
# ::{note}
# We have already set the minio credentials in the agent by default. If you test the sandbox example locally, you will need to set the AWS credentials in your environment variables.
# ```{prompt} bash
# export FLYTE_AWS_ENDPOINT="http://localhost:30002"
# export FLYTE_AWS_ACCESS_KEY_ID="minio"
# export FLYTE_AWS_SECRET_ACCESS_KEY="miniostorage"
# :::
# %%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you may have to tweak the formatting to get the code snippet to appear correctly in the note -- I just started, and am not entirely familiar with how these docs are formatted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neverett Please review again! Thanks really much!
Signed-off-by: Future Outlier <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Future-Outlier the documentation looks good to me! I'll leave it to @pingsutw for final approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @Future-Outlier @neverett
…into sensor-agent-doc-v2
Signed-off-by: Future Outlier <[email protected]>
Head branch was pushed to by a user without write access
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@neverett Can you help us merge this PR? |
@Future-Outlier I just approved this PR, so it should be able to be merged now! |
@neverett Do you know how to solve the sphinx timeout error? |
TL;DR
Enable users to use the sensor agent.
Senso Index: https://flyte--1180.org.readthedocs.build/projects/cookbook/en/1180/auto_examples/sensor/index.html
How to use sensor?: https://flyte--1180.org.readthedocs.build/projects/cookbook/en/1180/auto_examples/sensor/sensor.html
Tracking issue
#1173
flyteorg/flyte#4195
flyteorg/flyte#4235
flyteorg/flyte#3936
Screenshots