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 additional documentation for the with_overrides feature #1181

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dado5688
Copy link

@dado5688 dado5688 commented Oct 12, 2023

TL;DR

Enable users to understand how with_overrides can be used in dynamically update various task configurations

Complete description

Add a new example in Using with_overrides section

Tracking Issue

Fixes flyteorg/flyte#4067

Follow-up issue

NA

requests=Resources(cpu="1", mem="200Mi"),
limits=Resources(cpu="2", mem="350Mi"),
)
def run_tfjob() -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

This task shouldn't just return a string — it should actually showcase a TF operation.

Copy link
Author

Choose a reason for hiding this comment

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

Could you please explain it? If I understand correctly, are you suggesting that I should providing a detailed description of the entire TensorFlow training code, as this might overshadow the importance of discussing the with_overrides method? or can I provide a link to this file that explains the TensorFlow processing, allowing us to focus on how to override the task_conf and how to run it? I want to emphasize that our primary focus here is on demonstrating the usage of overrides, or what my thought that is wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Could you then include this as part of a note but not as a code block? Not a code block because we aren't including a full-fledged code snippet.

return run_tfjob().with_overrides(task_config=TfJob(
num_workers=num_workers,
num_ps_replicas=1,
num_chief_replicas=1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Include a new line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please format the code using black and isort.

@samhita-alla
Copy link
Contributor

You shouldn't remove the existing example. Please update the doc by adding a new one to it.

@dado5688
Copy link
Author

ok, I see!
I will rollback the existing example and adding a new one.

limits=resources,
container_image=custom_image,
)
def mnist_tensorflow_job(hyperparameters: Hyperparameters) -> training_outputs:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a simpler task. Let's not make this complicated, and every task has to have a definition.

@samhita-alla
Copy link
Contributor

@dado5688, just added a comment. Is it possible for you to take a look at them and incorporate the changes?

@dado5688
Copy link
Author

@samhita-alla sure! I still trying to write a simpler tf job for example.

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, @dado5688 could you resolve the merge conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] add additional documentation for the with_overrides feature
3 participants