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

Add JavaDoc comments to flytekit-api module #186

Merged
merged 3 commits into from
Feb 2, 2023
Merged

Conversation

pablocasares
Copy link
Collaborator

@pablocasares pablocasares commented Feb 2, 2023

Add JavaDoc comments to flytekit-api module following the protos in flyteidl-protos module

Type

  • Documentation
  • Bug Fix
  • Feature
  • Plugin

@pablocasares pablocasares requested a review from narape February 2, 2023 13:05
@pablocasares pablocasares force-pushed the java-doc-flytekit-api branch from d41b561 to c60fade Compare February 2, 2023 13:10
@@ -39,14 +42,14 @@ public abstract class DynamicJobSpec {
public abstract List<Binding> outputs();

/**
* Returns sub-workflows templates.
* [Optional] A complete list of task specs referenced in nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check here, this doesnät looks like it belongs to subworkflows

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @narape, I checked both the proto from flyteidl-protos and the original proto from here and it seems that the comment in flytekit-java is following the one in flyteidl repo. Do you think we should change it to something different?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a copy paste error (in the proto) of the field bellow. Keep the comment that it was before

Signed-off-by: Pablo Casares Crespo <[email protected]>
Signed-off-by: Pablo Casares Crespo <[email protected]>
@narape narape merged commit a1d5226 into master Feb 2, 2023
@narape narape deleted the java-doc-flytekit-api branch February 2, 2023 14:09
@@ -24,17 +24,32 @@
@AutoValue
public abstract class Container {

/**
* Command to be executed, if not provided, the default entrypoint in the container image will be
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: to avoid misinterpretations

Suggested change
* Command to be executed, if not provided, the default entrypoint in the container image will be
* Command to be executed. If not provided, the default entrypoint in the container image will be

public class ContainerError extends RuntimeException {

private static final long serialVersionUID = 5162780469952221158L;
/** A simplified code for errors, so that we can provide a glossary of all possible errors. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there known strings that we know the full list of them? If so, can we add a link to that list?

@@ -28,13 +28,19 @@ public static Builder builder() {

@AutoValue.Builder
public abstract static class Builder {

/**
* Name of the domain the resource belongs to. A domain can be considered as a subset within a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Name of the domain the resource belongs to. A domain can be considered as a subset within a
* Name of the domain the launch plan belongs to. A domain can be considered as a subset within a

(same below)

public abstract String project();

/**
* User provided value for the resource. The combination of project + domain + name uniquely
* identifies the resource. [Optional] - in certain contexts - like 'List API', 'Launch plans'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* identifies the resource. [Optional] - in certain contexts - like 'List API', 'Launch plans'
* identifies the resource. [Optional] - used in certain contexts - like 'List API', 'Launch plans'

public abstract List<Binding> inputs();

/**
* [Optional] Specifies execution dependency for this node ensuring it will only get scheduled to
* run after all its upstream nodes have completed. This node will have an implicit dependency on
Copy link
Collaborator

Choose a reason for hiding this comment

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

This node will have an implicit dependency on any node that appears in inputs field. Maybe this should be a comment for inputs() instead?

/**
* A predefined yet extensible Task type identifier. This can be used to customize any of the
* components. If no extensions are provided in the system, Flyte will resolve this task to its
* TaskCategory and default the implementation registered for the TaskCategory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't fully understand this part: "If no extensions are provided in the system, Flyte will resolve this task to its

  • TaskCategory and default the implementation registered for the TaskCategory."

public abstract Struct custom();

/**
* Indicates whether the system should attempt to lookup this task's output to avoid duplication
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also make it possible and say to re-use existing data 🙂 (or add both)

public abstract LiteralType literalType();

/** [Optional] string describing input variable. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Variable always used to refer to an input?

@pablocasares pablocasares restored the java-doc-flytekit-api branch February 2, 2023 14:57
@pablocasares pablocasares mentioned this pull request Feb 2, 2023
4 tasks
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