Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Flyte CoPilot now available for all containers #64

Merged
merged 4 commits into from
May 17, 2020
Merged

Conversation

kumare3
Copy link
Contributor

@kumare3 kumare3 commented May 14, 2020

TL;DR

Flyte CoPilot is available for all containers instead of a plugin.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Refer to the issue and associated design

Tracking Issue

flyteorg/flyte#297

Follow-up issue

NA

@kumare3 kumare3 requested review from EngHabu and wild-endeavor May 14, 2020 04:59
protos/flyteidl/core/tasks.proto Outdated Show resolved Hide resolved
protos/flyteidl/core/tasks.proto Outdated Show resolved Hide resolved
protos/flyteidl/core/tasks.proto Outdated Show resolved Hide resolved
protos/flyteidl/core/tasks.proto Show resolved Hide resolved
protos/flyteidl/core/tasks.proto Show resolved Hide resolved
protos/flyteidl/core/tasks.proto Show resolved Hide resolved
setup.py Outdated
@@ -1,6 +1,6 @@
from setuptools import setup, find_packages

__version__ = '0.17.30'
__version__ = '0.18.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a breaking change... let's keep it at 17.

Suggested change
__version__ = '0.18.0'
__version__ = '0.17.31'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but, if you want to use data loading, you need a min version. That is why i was thinking of upping the minor?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm if you want to use data loading, you must be on 0.17.31+, same thing, right? we should really move to v1 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya soon :)

// Flyte CoPilot, eliminates the needs of flytekit or sdk inside the container. Any inputs required by the users container are side-loaded in the input_path
// Any outputs generated by the user container - within output_path are automatically uploaded.
message DataLoadingConfig {
// MetadataFormat decides the encoding format in which the input metadata should be made available to the containers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be nitpicky, but I've read these comments three times now and I'm still confused. What is MetadataFormat? The comment says "... format in which the input metadata...". What is input metadata and how is it different than input? It sounds like it's the same thing. If it's an input, can we not just call it "Input"? Currently the situation makes it sound like: 1) there's input data; 2) there's metadata about that input; 3) the format of that metadata can be configured; but 4) the format of the input itself cannot be configured.

Sorry to be annoying but this is the IDL - names here stick around forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we just need to write better documentation about metadata. as this is not really data in all cases.

@kumare3 kumare3 merged commit f818179 into master May 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants