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

Raw output prefix #34

Merged
merged 8 commits into from
Aug 27, 2020
Merged

Raw output prefix #34

merged 8 commits into from
Aug 27, 2020

Conversation

wild-endeavor
Copy link
Contributor

No description provided.

@@ -20,7 +20,8 @@ Note this currently does not include articles/tid-bits on how to use the Flyte p
1. [Dynamic Tasks](recipes/dynamictasks)
1. [Tasks without flytekit or Using arbitrary containers](recipes/rawcontainers)
1. [Jupyter notebook as a task](recipes/notebook_tasks)
1. [Different container per task](recipes/differentcontainers)
1. [Different container per task](recipes/differentcontainers)
1. [Customize Offloaded Data Location](recipes/offloaded_output_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of this, can we just make this work for all examples?

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh nevermind, I get it now, we are saying that all outputs will be in default storage location and you can customize if you want


[platform]
url=host.docker.internal:30081
insecure=True

# Deprecated. Please use the raw_output_data_prefix above
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to delete the current config objects, then I need to add more code to the flytekit pr. Specifically, the new raw output config object is not currently used at run time, it’s only used at compile time (sent up to admin at launch plan registration).

Currently in that PR, if it's missing at runtime, it'll fall back to the current aws_shard/gcs_prefix config objects. If we want to remove the old configs entirely, we'll have to make it so that if Admin doesn't send it (because users are on an older version of admin), we do our own sharding based off of the raw output config at runtime, when click starts up for instance.

However, if users update flytekit, but don't add a raw output config and we remove the existing config objects, then at runtime, it won't know where to write to.

So we can,

  1. We can just add runtime deprecation notices and delete them later.
  2. Add a fallback at registration time to the existing shard formatter/prefix if the raw output prefix is missing.

I actually think I'd go with option 1 for now, at least until the different sharding methods you just added to flyteplugins are available for picking and choosing. I hate assuming migration timelines though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant delete it in the example? Set good precedence

Copy link
Contributor

Choose a reason for hiding this comment

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

talked offline, I understand the reason is for backwards compatibility - Someone may run the example on an older Flyte installation.

@kumare3
Copy link
Contributor

kumare3 commented Aug 27, 2020

I think we should update flytesnacks/python examples too

@kumare3 kumare3 self-requested a review August 27, 2020 18:02
kumare3
kumare3 previously approved these changes Aug 27, 2020
@wild-endeavor wild-endeavor merged commit e0a92ff into master Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants