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

Upload to s3 feature added into interactive workflow #84

Closed
wants to merge 12 commits into from

Conversation

Sahiler
Copy link
Contributor

@Sahiler Sahiler commented Apr 17, 2024

Moved away from using snowflake for the interactive workflow example as the output can often be unstructured text.

Two main reasons:

  1. This made it difficult to create a snowpark sql query dynamically.
  2. Uploading to s3 would be the valid approach to house any unstructured results and add onto the aws flavored demos.

Next steps

I have a generate_suggested_file_name task that is currently a WIP, where currently the file name that is used is static in the rest of the workflow. I plan on debugging this flaky task, but currently the script is valid and demo-able. Additionally, will point the .deploy command to the prefect-demos repo main branch once the updated flow code gets merged in.

@Sahiler Sahiler requested a review from a team as a code owner April 17, 2024 19:03
@masonmenges
Copy link
Collaborator

masonmenges commented Apr 17, 2024

@Sahiler Make sure you run the pre-commits in you're local environment, i.e. pip install pre-commit, then pre-commit run, I'll get that added to the requirements file for the demos repo so this triggers automatically when you add a git commit

Copy link
Collaborator

@masonmenges masonmenges left a comment

Choose a reason for hiding this comment

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

Make sure the pre-commits are run, we'll probably hold off on merging this to main I do have a Dev branch that we'll probably want to merge this to first for testing in the se-dev workspace, still need to make sure the rules for the dev workspace are setup correctly though

@@ -20,6 +20,7 @@ Interactive Workflow Demo is designed to guide you through the process of settin
Before starting, ensure you have the following installed:
- Python 3.6+
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be 3.8 since that's the lowest version I think we support now

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably assume that the user here knows that this requires Python. Do we even need a pre-requisites section?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's probably fair, I think in general we should assume relevant information for these demos outside of perfect specific functionality is familiar to whoever might look at it

from prefect import flow

flow.from_source(
source="https://github.com/Sahiler/interactive-workflow-demo.git",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll want to probably set this up in the dev branch for this repo first to do some fine tuning and then create/test the deployment here https://app.prefect.cloud/account/0ff44498-d380-4d7b-bd68-9b52da03823f/workspace/f579e720-7969-4ab8-93b7-2dfa784903e6/dashboard to start with before we merge into main

flows/aws/interactive-workflow/interactive-workflow.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@EmilRex EmilRex left a comment

Choose a reason for hiding this comment

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

Left some comments.

One bigger point is that reading over the README, it's not clear what this example is trying to accomplish. I understand what features are being used, but not what is happening, nor why I would use this pattern.

flows/aws/interactive-workflow/README.md Outdated Show resolved Hide resolved
flows/aws/interactive-workflow/interactive-workflow.py Outdated Show resolved Hide resolved
flows/aws/interactive-workflow/README.md Outdated Show resolved Hide resolved
@@ -20,6 +20,7 @@ Interactive Workflow Demo is designed to guide you through the process of settin
Before starting, ensure you have the following installed:
- Python 3.6+
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably assume that the user here knows that this requires Python. Do we even need a pre-requisites section?

flows/aws/interactive-workflow/README.md Show resolved Hide resolved
flows/aws/interactive-workflow/README.md Outdated Show resolved Hide resolved
flows/aws/interactive-workflow/marvin_extension.py Outdated Show resolved Hide resolved
flows/aws/interactive-workflow/marvin_extension.py Outdated Show resolved Hide resolved
@Sahiler
Copy link
Contributor Author

Sahiler commented Apr 18, 2024

Have a few readme updates and new s3 function to add in, converting to a draft in the meantime

@Sahiler Sahiler marked this pull request as draft April 18, 2024 15:31
@EmilRex EmilRex closed this Apr 22, 2024
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