flytekit icon indicating copy to clipboard operation
flytekit copied to clipboard

Show version and execute by version

Open novahow opened this issue 1 year ago • 10 comments

Tracking issue

fixes https://github.com/flyteorg/flyte/issues/4707

Why are the changes needed?

Show versions and creation time of remote-task/launchplan/workflow

What changes were proposed in this pull request?

This PR contains 2 parts. For the show-versions part, I created a new top-level command VersionCommand which inherits the RunCommand class since they function similarly. Also, RemoteEntityVersionGroup inherits RemoteEntityGroup,and DynamicEntityVersionCommand inherits both click.RichGroup and DynamicEntityLaunchCommand. It inherits click.RichGroup because we want to display versions and creation time with click automatically. We also extract parts of RunLevelParams to create a base param class RunBaseParams, so that VersionLevelParams which inherits RunBaseParams can display only the options that will be used is show-version command.

To fetch the creation time of the task, the created_at attribute is added in closures. This ensures that both versions and created_time can be retrieved from list_tasks_paginated method.

@classmethod
    def from_flyte_idl(cls, p):
        """
        :param flyteidl.admin.workflow_pb2.WorkflowClosure p:
        :rtype: WorkflowClosure
        """
        return cls(compiled_workflow=_compiler_models.CompiledWorkflowClosure.from_flyte_idl(p.compiled_workflow))
        return cls(
            compiled_workflow=_compiler_models.CompiledWorkflowClosure.from_flyte_idl(p.compiled_workflow),
            created_at=p.created_at.ToDatetime().replace(tzinfo=_timezone.utc) if p.HasField("created_at") else None,
        )

For the execute with version part, I simply split the entity name with ":", and iteratively loop through possible combinations of entity_name and version with the _looped_fetch_entity method.

How was this patch tested?

By running pyflyte show-versions remote-[launchplan|workflow|task] {name} , the versions will be displayed Code snippet:

from math import sqrt
from flytekit import workflow, task, LaunchPlan
from typing import List, Union
import random
import pdb

@task
def standard_deviation(values: List[float], mu: float) -> float:
    variance = sum([(x - mu) ** 2 for x in values])
    return sqrt(variance)

@task
def standard_scale(values: List[float], mu: float, sigma: float) -> List[float]:
    return [(x - mu) / sigma for x in values]


@task
def mean(values: List[float]) -> float:
    return sum(values) / len(values)

@task
def mean2(values: List[float]) -> float:
    return sum(values) / len(values)

@task
def generate_data(num_samples: int, seed: int) -> List[float]:
    random.seed(seed)
    return [random.random() for _ in range(num_samples)]


@workflow
def standard_scale_workflow(values: List[float]) -> List[float]:
    mu = mean(values=values)
    sigma = standard_deviation(values=values, mu=mu)
    return standard_scale(values=values, mu=mu, sigma=sigma)


@workflow
def workflow_with_subworkflow(num_samples: int, seed: int) -> List[float]:
    data = generate_data(num_samples=num_samples, seed=seed)
    return standard_scale_workflow(values=data)


# pdb.set_trace()
print("Compiled")
# LaunchPlan.CACHE.pop("standard_scale_lp", None)
standard_scale_launch_plan = LaunchPlan.get_or_create(
    standard_scale_workflow,
    name="standard_scale_lp",
    default_inputs={"values": [3.0, 4.0, 5.0]}
)

Example:

pyflyte show-versions remote-task flyteTest.example.mean

One can then run pyflyte run remote-task flyteTest.example.mean:{version} --inputs ...

Setup process

Screenshots

image image image image

Check all the applicable boxes

  • [ ] I updated the documentation accordingly.
  • [v] All new and existing tests passed.
  • [v] All commits are signed-off.

Related PRs

Docs link

novahow avatar Jan 21 '24 11:01 novahow

There is one problem with this --show-versions implies you cannot have a task or a workflow with show_versions as an input Example

@task 
def do(show_versions: bool):
    ...

which might be ok. But to counter this, I suggested add a subcommand of type

pyflyte run remote-task mytask show-versions

Note the missing --

cc @wild-endeavor what should we do?

If you create a subcommand, like how we do for remote-entities. the table also does not need to be created!

kumare3 avatar Jan 26 '24 01:01 kumare3

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.22%. Comparing base (66a6018) to head (5556d00).

:exclamation: Current head 5556d00 differs from pull request most recent head 68884bd. Consider uploading reports for the commit 68884bd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2124      +/-   ##
==========================================
- Coverage   83.48%   83.22%   -0.27%     
==========================================
  Files         324      300      -24     
  Lines       24744    23938     -806     
  Branches     3521     3502      -19     
==========================================
- Hits        20658    19922     -736     
+ Misses       3453     3388      -65     
+ Partials      633      628       -5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 26 '24 01:01 codecov[bot]

can we make show-versions or maybe just versions a top level command on pyflyte? so just pyflyte versions instead of pyflyte run?

wild-endeavor avatar Jan 26 '24 01:01 wild-endeavor

can we make show-versions or maybe just versions a top level command on pyflyte? so just pyflyte versions instead of pyflyte run?

Thanks for the advice, this sounds more feasible for me, I've modified my PR to implement this behavior.

novahow avatar Jan 26 '24 09:01 novahow

I can't get this to run locally actually. running the new command I'm getting

  File "/Users/ytong/go/src/github.com/flyteorg/flytekit/flytekit/clis/sdk_in_container/versions.py", line 19, in __init__
    super(click.RichGroup, self).__init__(name, h, entity_name, launcher, **kwargs)

related, why is DynamicEntityVersionCommand inheriting both click.RichGroup and DynamicEntityLaunchCommand? why is this related to launch?

wild-endeavor avatar Feb 13 '24 20:02 wild-endeavor

@wild-endeavor Hi, sorry for the confusion. DynamicEntityVersionCommand inherits the DynamicEntityLaunchCommand because it needs to call the _fetch_entity method of DynamicEntityLaunchCommand in list_commands method, so that it can find the type of the desired entity. Regarding the error, could you please provide the command you used that triggers the error you mentioned as well as the error log? Thanks

novahow avatar Feb 14 '24 12:02 novahow