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

Implement proper custom typing in Flytekit to improve UX and visibility in UI for complex classes such as HyperparameterTuningJobConfig #455

Closed
bnsblue opened this issue Aug 5, 2020 · 2 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@bnsblue
Copy link
Contributor

bnsblue commented Aug 5, 2020

Currently users need to call to_flyte_idl() for complex classes such as HyperparameterTuningJobConfig in user code, which is not ideal:

hpo_inputs={
    ...
    "hyperparameter_tuning_job_config": HyperparameterTuningJobConfig(
        hyperparameter_ranges=ParameterRanges(
            parameter_range_map={
                ....
            }
        ),
        tuning_strategy=HyperparameterTuningStrategy.BAYESIAN,
        tuning_objective=HyperparameterTuningObjective(
            ....
        ),
        training_job_early_stopping_type=TrainingJobEarlyStoppingType.AUTO
    ).to_flyte_idl()
   ...
}

We should implement proper custom typing in Flytekit to get rid of this.

Two approaches

  1. Create my own type and serialize it

Int, Collection, String => return a struct (proto struct)

  1. Use proto as binary

Take a proto -> serialize it
binary->struct

@EngHabu EngHabu added this to the 0.7.0 milestone Aug 5, 2020
@bnsblue bnsblue added the enhancement New feature or request label Aug 5, 2020
@kumare3 kumare3 modified the milestones: 0.7.0, 0.8.0 Sep 2, 2020
@bnsblue bnsblue assigned EngHabu and unassigned bnsblue Sep 9, 2020
@EngHabu
Copy link
Contributor

EngHabu commented Sep 21, 2020

  • Introduce SDK Proto type (similar to Types.Proto) that serializes protos into structs and can be used by users for any proto.
  • Migrate HypeparameterTuningJobConfig type to using the struct version to enable visualization of inputs in Flyte UI
  • Make a corresponding change in HPO Golang plugin to fallback to binary proto deserialization for backward compat.
  • Override call function in HPO Task to clear up the usage docs so that IDEs do not infer the wrong signature and complain about passed inputs.
  • Move parameter ranges out of the config object as inputs of type Proto

@EngHabu
Copy link
Contributor

EngHabu commented Sep 30, 2020

Flytekit:

  • Introduce Types.GenericProto to represent Protos using structs (for better Fyte Console Visibility)
  • Use structs for custom sagemaker types (instead of binary protos)
  • Accept FlyteIdlEntity in Types.GenericProto constructor to get rid of .to_flyte_idl() requirement
  • Use Types.GenericProto to represent ParameterRange proto as a Flyte Type that can be used for inputs/outputs
  • Introduce tunable_params for HPO job, the list of params will then exist as explicit inputs of the generated HPO job.
  • Override call function in HPO Task to clear up the usage docs so that IDEs do not infer the wrong signature and complain about passed inputs.
  • Update and fix unit tests
  • Update a functional test WF to use the new pattern and verify an end-2-end WF works.

HPO Go-Plugin:

  • Accept struct input and fallback to binary proto (for backward compat)
  • Parse other inputs of type ParameterRange and pass them to HPO jobs as hyper parameters

@anandswaminathan anandswaminathan modified the milestones: 0.8.0, 0.9.0 Sep 30, 2020
@EngHabu EngHabu closed this as completed Nov 4, 2020
eapolinario pushed a commit to eapolinario/flyte that referenced this issue Dec 6, 2022
* fix: Dockerfile to reduce vulnerabilities

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update builder image as well

Signed-off-by: Haytham Abuelfutuh <[email protected]>

Co-authored-by: Haytham Abuelfutuh <[email protected]>
eapolinario pushed a commit to eapolinario/flyte that referenced this issue Dec 20, 2022
* fix: update node executions to display map tasks
* fix: update map task logs styles
* test: add/update unit tests
* fix: fix flickering and unnecessary re-renders

Signed-off-by: Olga Nad <[email protected]>
eapolinario pushed a commit to eapolinario/flyte that referenced this issue Aug 9, 2023
* fix: Dockerfile to reduce vulnerabilities

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update builder image as well

Signed-off-by: Haytham Abuelfutuh <[email protected]>

Co-authored-by: Haytham Abuelfutuh <[email protected]>
eapolinario pushed a commit to eapolinario/flyte that referenced this issue Apr 30, 2024
Signed-off-by: Niels Bantilan <[email protected]>
eapolinario pushed a commit to eapolinario/flyte that referenced this issue Apr 30, 2024
Signed-off-by: Niels Bantilan <[email protected]>
austin362667 pushed a commit to austin362667/flyte that referenced this issue May 7, 2024
robert-ulbrich-mercedes-benz pushed a commit to robert-ulbrich-mercedes-benz/flyte that referenced this issue Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants