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

Get full inputs/outputs from execution data #92

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

kanterov
Copy link
Contributor

@kanterov kanterov commented Aug 31, 2020

TL;DR

Fixes Input/Output view when using GCS.

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

flyteadmin doesn't know how to return public URLs for GCS objects. It returns URL in format gs://... that doesn't work. This fixes Input/Output view when using GCS for Storage by using new fullInputs/fullOutputs fields in getExecutionData proto. Change is backward-compatible, if full inputs/outputs aren't present, it's going to fallback on the existing mechanism.

I had to take 6a3b80f and few other commits because I needed new fields in flyteidl.

Tracking Issue

flyteorg/flyte#444

Follow-up issue

NA

@kanterov
Copy link
Contributor Author

cc @kumare3

@kumare3
Copy link
Contributor

kumare3 commented Aug 31, 2020

@kanterov today you showed that you know typescript 😜. But why not a separate PR

@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2020

Codecov Report

Merging #92 into master will increase coverage by 0.79%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   66.70%   67.49%   +0.79%     
==========================================
  Files         370      371       +1     
  Lines        5962     6009      +47     
  Branches      930      938       +8     
==========================================
+ Hits         3977     4056      +79     
+ Misses       1985     1953      -32     
Impacted Files Coverage Δ
...Details/ExecutionNodeDetails/OutputNodeDetails.tsx 0.00% <ø> (ø)
.../Executions/ExecutionDetails/NodeExecutionData.tsx 0.00% <0.00%> (ø)
...ponents/Executions/ExecutionInputsOutputsModal.tsx 39.13% <0.00%> (ø)
...xecutions/ExecutionDetails/NodeExecutionInputs.tsx 54.54% <100.00%> (ø)
...ecutions/ExecutionDetails/NodeExecutionOutputs.tsx 54.54% <100.00%> (ø)
src/components/Literals/RemoteLiteralMapViewer.tsx 94.44% <100.00%> (+44.44%) ⬆️
src/components/common/constants.ts 100.00% <0.00%> (ø)
...omponents/Executions/ExecutionDetails/constants.ts 100.00% <0.00%> (ø)
src/components/common/MoreOptionsMenu.tsx 100.00% <0.00%> (ø)
src/components/Executions/utils.ts 76.69% <0.00%> (+2.91%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb200bd...f3012ca. Read the comment docs.

@schottra
Copy link
Contributor

@kanterov Thanks for doing this!

The other PR should be merged today and you can rebase off of master after that. Or you could just update the version of flyteidl in package.json and run yarn to update the lock file instead of pulling in all of the other changes.

Would you mind writing/updating tests for the components that you changed to make sure they use the new data when it's available and fall back to the old data otherwise?

@kanterov kanterov force-pushed the use-full-inputs-outputs branch from b22923f to f3012ca Compare August 31, 2020 18:30
@kanterov
Copy link
Contributor Author

@schottra done. Some non-code related builds checks are failing, it seems I need to configure something for them to work. Do I need to fix those?

@schottra
Copy link
Contributor

@kanterov I think the build/push actions are expected to fail, since they run from a fork and you won't have permissions to push to github/dockerhub. I should update those to not run on forked repositories.
However, the CLA check needs to be addressed before we merge. Would you mind clicking through on that and signing the contributor agreement if you haven't already?
If you have, then the email address associated with your commits also needs to be the one associated with your github account, so that the check can match them.

@schottra
Copy link
Contributor

schottra commented Sep 2, 2020

@kanterov If you rebase on latest master, I've updated the Github actions, so two of those failed checks should now pass.

@kanterov kanterov closed this Sep 17, 2020
@kanterov kanterov reopened this Sep 17, 2020
@kanterov
Copy link
Contributor Author

@schottra sorry it took some time, but now everything is done :) please take a look again

@service-github-lyft-semantic-release
Copy link
Contributor

🎉 This PR is included in version 0.12.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

5 participants