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

Sensor Agent Doc Integration Example #1180

Merged
merged 32 commits into from
Dec 6, 2023

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Oct 11, 2023

Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier Future-Outlier changed the title Sensor Agent Doc [Reupload] Sensor Agent Doc Integration Example Nov 7, 2023
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved

```
pyflyte run --remote \
https://raw.githubusercontent.com/flyteorg/flytesnacks/master/examples/sensor/sensor/sensor.py wf
Copy link
Member

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...

Copy link
Contributor

@neverett neverett Nov 13, 2023

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.

Copy link
Contributor

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?

examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
examples/sensor/sensor/sensor.py Outdated Show resolved Hide resolved
Future-Outlier and others added 13 commits November 8, 2023 16:33
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]>
@pingsutw
Copy link
Member

@neverett mind taking a look when you get a chance, thanks

Copy link
Contributor

@neverett neverett left a 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.
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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"
# :::
# %%

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

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]>
Copy link
Contributor

@neverett neverett left a 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.

Copy link
Member

@pingsutw pingsutw left a 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

pingsutw
pingsutw previously approved these changes Nov 21, 2023
@pingsutw pingsutw enabled auto-merge (squash) November 21, 2023 00:58
Future Outlier added 2 commits November 28, 2023 11:41
auto-merge was automatically disabled November 28, 2023 03:46

Head branch was pushed to by a user without write access

Signed-off-by: Future Outlier <[email protected]>
pingsutw
pingsutw previously approved these changes Nov 28, 2023
Signed-off-by: Future Outlier <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier Future-Outlier enabled auto-merge (squash) December 2, 2023 04:00
@Future-Outlier
Copy link
Member Author

@neverett Can you help us merge this PR?
Also, can you help us remove the request changed?
This can help us auto-merge this PR, thank you really much!

@neverett
Copy link
Contributor

neverett commented Dec 4, 2023

@Future-Outlier I just approved this PR, so it should be able to be merged now!

@Future-Outlier
Copy link
Member Author

@neverett Do you know how to solve the sphinx timeout error?
I think the error is not related to the sensor doc.
Thanks a lot!

@Future-Outlier Future-Outlier merged commit db817e8 into flyteorg:master Dec 6, 2023
86 of 87 checks passed
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