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

[Docs] Added doc on how to handle a large response when using FlyteRemote.sync #1212

Merged
merged 8 commits into from
Oct 7, 2022

Conversation

Ln11211
Copy link
Contributor

@Ln11211 Ln11211 commented Oct 2, 2022

TL;DR

Updated control_plane.rst doc

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Added about large response while using Remote.Sync and how to handle it in the control_plane.rst doc. This PR is in reference to flyteorg/flyte#2932 (comment) and issue #2809.

Tracking Issue

flyteorg/flyte#2809

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

@welcome
Copy link

welcome bot commented Oct 2, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@Ln11211
Copy link
Contributor Author

Ln11211 commented Oct 2, 2022

@samhita-alla Hey I mentioned about the large response just like you asked. Check from Line 302

Do you want any changes? :)

eapolinario and others added 5 commits October 2, 2022 17:08
* Open HashMethod to all types

* Fix test in test_type_hints.py

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: LN <[email protected]>
…ies (flyteorg#1192)

The following vulnerabilities are fixed by pinning transitive dependencies:
- https://snyk.io/vuln/SNYK-PYTHON-PROTOBUF-3031740

Signed-off-by: LN <[email protected]>
* pyflyte run non-fast register

Signed-off-by: Kevin Su <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: LN <[email protected]>
Added about large response  while using Remote.Sync() and how to handle it in the doc

Signed-off-by: LN <[email protected]>
@samhita-alla
Copy link
Contributor

@Ln11211, I see multiple files that have already been merged into the master branch. Please ensure only the file of concern is present by rebasing the branch.

@Ln11211
Copy link
Contributor Author

Ln11211 commented Oct 4, 2022

@Ln11211, I see multiple files that have already been merged into the master branch. Please ensure only the file of concern is present by rebasing the branch.

I don't understand what happened, I synced my fork and then used the git rebase HEAD~5 --signoff and git push --force-with-lease origin <your-branch> to sign off my commits. I guess I messed up the second git command.. How do I rebase the branch?

@Ln11211
Copy link
Contributor Author

Ln11211 commented Oct 4, 2022

Here is what I think I'm supposed to do--
I'll discard my changes(previous commits) and then sync the fork and then commit the changes again, this will fix the issue right?

@samhita-alla
Copy link
Contributor

Hopefully! If that doesn't work, please create a new PR.

@Ln11211
Copy link
Contributor Author

Ln11211 commented Oct 4, 2022

@samhita-alla Good news, All I had to do was update my fork and pull all new commits and that fixed the problem, I feel relieved that the problem got solved easily...

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #1212 (976803c) into master (0bd3261) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1212   +/-   ##
=======================================
  Coverage   68.57%   68.57%           
=======================================
  Files         288      288           
  Lines       26225    26225           
  Branches     2929     2929           
=======================================
  Hits        17985    17985           
  Misses       7762     7762           
  Partials      478      478           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Ln11211 and others added 2 commits October 6, 2022 23:23
updating FlyteRemote sync message

Co-authored-by: Samhita Alla <[email protected]>
Updated the fix for the error and also removed the repeated error message line.

Signed-off-by: LN <[email protected]>
@eapolinario eapolinario merged commit 92ef28e into flyteorg:master Oct 7, 2022
@welcome
Copy link

welcome bot commented Oct 7, 2022

Congrats on merging your first pull request! 🎉

@samhita-alla
Copy link
Contributor

@Ln11211, seems like the content's not rendering properly. Would you mind creating a flytekit PR to remove the To fix this error, ... bullet point and have troubleshooting guide instead of flyte: in refer to the flyte: statement in the note.

image

@Ln11211
Copy link
Contributor Author

Ln11211 commented Oct 7, 2022

@Ln11211, seems like the content's not rendering properly. Would you mind creating a flytekit PR to remove the To fix this error, ... bullet point and have troubleshooting guide instead of flyte: in refer to the flyte: statement in the note.

image

Yup. I'll do it right now

@Ln11211 Ln11211 mentioned this pull request Oct 7, 2022
8 tasks
@Ln11211
Copy link
Contributor Author

Ln11211 commented Oct 7, 2022

@Ln11211, seems like the content's not rendering properly. Would you mind creating a flytekit PR to remove the To fix this error, ... bullet point and have troubleshooting guide instead of flyte: in refer to the flyte: statement in the note.

image

Here it is #1229 @samhita-alla

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.

6 participants