Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

Added fast register functionality #112

Merged
merged 22 commits into from
Jul 1, 2021
Merged

Added fast register functionality #112

merged 22 commits into from
Jul 1, 2021

Conversation

yindia
Copy link
Contributor

@yindia yindia commented Jun 22, 2021

TL;DR

  • Added unit test fo register example (Currently there one todo that will fix after flytesnack ci issue got fixed)
  • Added fast registration functionality

Test Cases :

  • Register on fast serialize proto without addition flags
$  flytectl  register files /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --continueOnError -c config.yaml --version=v1.0.0 
you are trying to register fast serialize workflow. Please pass additional flags like --additionalDistributionDir 
  • Register on invalid fles
$  flytectl  register files /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --continueOnError -c config.yaml --version=v1.0.0 
input package have some invalid files. try to run pyflyte package again
  • Register with invalid storage client
$  flytectl  register files /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --continueOnError -c config.yaml --version=v1.0.0 
please check your Storage Config. It failed while uploading the source code
  • Register on fast serialize proto without source code
$  flytectl  register files /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --continueOnError -c config.yaml --version=v1.0.0 
please check your fast serialize package, There is no source code available but you passed additional flag --additionalDistributionPath
  • Register on Invalid tar
$  flytectl  register files abc.tar  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --continueOnError -c config.yaml --version=v1.0.0 
input package have some invalid files. try to run pyflyte package again
  • Register on fast serialize proto (Successful execution of workflow)
21:10:38 ➜ flytectl  register files /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --additionalDistributionPath="s3://my-s3-bucket/fast" --continueOnError -c config.yaml --version=v1.0.0 
 ---------------------------------------------------------------------------------------------------------------------------------------- --------- ------------------------------
| NAME (84)                                                                                                                              | STATUS  | ADDITIONAL INFO              |
 ---------------------------------------------------------------------------------------------------------------------------------------- --------- ------------------------------
| /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/00_core.control_flow.dynamics.return_index_1.pb              | Success | Successfully registered file |
 -----------------------------------------------------------------------------------------------------------------------------
  • Register on serialize proto (Successful execution of workflow)
21:10:38 ➜ flytectl  register files /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --continueOnError -c config.yaml --version=v1.0.0 
 ---------------------------------------------------------------------------------------------------------------------------------------- --------- ------------------------------
| NAME (84)                                                                                                                              | STATUS  | ADDITIONAL INFO              |
 ---------------------------------------------------------------------------------------------------------------------------------------- --------- ------------------------------
| /Users/yuvraj/workspace/flyteorg/flytesnacks/cookbook/core/_pb_output/00_core.control_flow.dynamics.return_index_1.pb              | Success | Successfully registered file |
 -----------------------------------------------------------------------------------------------------------------------------

Flytectl Config

admin:
  # For GRPC endpoints you might want to use dns:///flyte.myexample.com
  endpoint: dns:///localhost:30081
  insecure: true
logger:
  show-source: true
  level: 3
storage:
  connection:
    access-key: minio
    auth-type: accesskey
    disable-ssl: true
    endpoint: http://localhost:9000
    region: my-region-here
    secret-key: miniostorage
  container: my-container
  type: minio

Testing steps

$ flytectl sandbox start  --flytesnacks={{PATH_OF_FLYTESNACK}}
$ cd {{PATH_OF_FLYTESNACK}}
$ make -C cookbook/core serialize 
$ flytectl  register files {{PATH_OF_FLYTESNACK}}/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"
$ RUN THE WORKFLOW IN CONSOLE
$ make some changes in task 
$ make -C cookbook/core fast_serialize 
$ flytectl register files {{PATH_OF_FLYTESNACK}}/cookbook/core/_pb_output/*  -d development  -p flytesnacks  --k8ServiceAccount=default --outputLocationPrefix="s3://my-s3-bucket/prefix"  --additionalDistributionDir="s3://my-s3-bucket/fast" --destinationDir="/usr/src"
$ RUN THE WORKFLOW IN CONSOLE

NOTE: Checks are failing because of flag changes.

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

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/lyft/flyte/issues/

Follow-up issue

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

Signed-off-by: Yuvraj <code@evalsocket.dev>
@codecov
Copy link

codecov bot commented Jun 22, 2021

Codecov Report

Merging #112 (bf750f3) into master (f6e4526) will increase coverage by 1.62%.
The diff coverage is 75.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   73.54%   75.17%   +1.62%     
==========================================
  Files         104      105       +1     
  Lines        2491     2578      +87     
==========================================
+ Hits         1832     1938     +106     
+ Misses        560      527      -33     
- Partials       99      113      +14     
Impacted Files Coverage Δ
cmd/register/register_util.go 79.93% <69.11%> (+5.96%) ⬆️
cmd/register/files.go 85.71% <89.47%> (-1.79%) ⬇️
...md/config/subcommand/register/filesconfig_flags.go 100.00% <100.00%> (ø)
cmd/config/subcommand/sandbox/config_flags.go 100.00% <100.00%> (+84.21%) ⬆️
cmd/register/examples.go 88.88% <100.00%> (+88.88%) ⬆️
cmd/sandbox/start.go 77.50% <100.00%> (ø)
cmd/sandbox/sandbox.go 100.00% <0.00%> (ø)
pkg/docker/sandbox_util.go
cmd/sandbox/exec.go 64.28% <0.00%> (ø)
pkg/docker/docker_util.go 90.21% <0.00%> (ø)
... and 2 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 f6e4526...bf750f3. Read the comment docs.

yindia added 2 commits June 22, 2021 17:21
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
@yindia yindia changed the title [WIP] Added fast register functionality Added fast register functionality Jun 23, 2021
@yindia yindia marked this pull request as ready for review June 23, 2021 15:43
@yindia yindia requested a review from pmahindrakar-oss June 23, 2021 15:43
@kumare3
Copy link
Contributor

kumare3 commented Jun 23, 2021

You do not need the cache configuration

@kumare3
Copy link
Contributor

kumare3 commented Jun 23, 2021

I see that fast is just a flag, what happens if fast is not set, but the output files are generated using fast serialization? Do we have some check that detects this and fails / asserts

@yindia
Copy link
Contributor Author

yindia commented Jun 23, 2021

@kumare3 we have that check. So if you try to register fast serialize files then it will through the error but currently, we don't have a check for fast registration of serialize proto (It should also through error. Right ?).

@yindia yindia force-pushed the fast-register branch 2 times, most recently from 0349cf5 to d290286 Compare June 23, 2021 21:31
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
cmd/register/examples.go Outdated Show resolved Hide resolved
cmd/register/examples.go Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
cmd/register/register_util.go Outdated Show resolved Hide resolved
cmd/register/register_util.go Outdated Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
cmd/config/subcommand/register/files_config.go Outdated Show resolved Hide resolved
Signed-off-by: Yuvraj <code@evalsocket.dev>
@yindia yindia requested a review from pmahindrakar-oss June 24, 2021 13:03
Signed-off-by: Yuvraj <code@evalsocket.dev>
cmd/register/files.go Outdated Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
yindia added 2 commits June 29, 2021 13:52
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
@yindia yindia requested a review from pmahindrakar-oss June 29, 2021 10:14
cmd/register/register_util.go Outdated Show resolved Hide resolved
cmd/register/register_util.go Outdated Show resolved Hide resolved
cmd/register/register_util.go Outdated Show resolved Hide resolved
cmd/register/register_util.go Outdated Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
kumare3
kumare3 previously approved these changes Jul 1, 2021
Copy link
Contributor

@pmahindrakar-oss pmahindrakar-oss left a comment

Choose a reason for hiding this comment

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

Doc fixes can be taken as followup

cmd/register/files.go Outdated Show resolved Hide resolved
cmd/register/files.go Outdated Show resolved Hide resolved
Signed-off-by: Yuvraj <code@evalsocket.dev>
@yindia yindia dismissed stale reviews from pmahindrakar-oss and kumare3 via f62639d July 1, 2021 13:11
Signed-off-by: Yuvraj <code@evalsocket.dev>
@pmahindrakar-oss pmahindrakar-oss dismissed EngHabu’s stale review July 1, 2021 13:30

The issue has been addressed by @yuvraj

@pmahindrakar-oss pmahindrakar-oss merged commit 67c6ff9 into master Jul 1, 2021
@pmahindrakar-oss pmahindrakar-oss deleted the fast-register branch July 1, 2021 13:52
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.

4 participants