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

Inconsistent ExitHandler behavior #1899

Closed
logicbomb421 opened this issue Aug 20, 2019 · 2 comments · Fixed by #2411
Closed

Inconsistent ExitHandler behavior #1899

logicbomb421 opened this issue Aug 20, 2019 · 2 comments · Fixed by #2411
Assignees

Comments

@logicbomb421
Copy link

logicbomb421 commented Aug 20, 2019

This is an expansion on #999.


What happened:
It appears currently the only valid way to create an ExitHandler is using dsl.ContainerOp, with is_exit_handler set to True. When the pipeline runs, duplicate nodes are spawned at the beginning of the workflow and execute the exit handler logic immediately. Attempting to set is_exit_handler manually on an ad-hoc component created with func_to_container_op, or one loaded via load_component[*], appears to do nothing.

What did you expect to happen:
Regardless of where the component came from, marking it as an exit handler should inform Kubeflow the component is an exit handler, and said handler should only be invoked once upon exiting the pipeline.

What steps did you take:

func_to_container_op exit handler

import kfp.dsl as dsl
import kfp.components as components

def print_message(message: str) -> None:
  print(message or 'NO MESSAGE SUPPLIED!')
print_op = components.func_to_container_op(print_message, base_image='python:3.7-alpine')

@dsl.pipeline(name='simple pipeline')
def pipeline_func():
  exit_handler = print_op('EXITED')
  exit_handler.is_exit_handler=True
  with dsl.ExitHandler(exit_handler):
    first = print_op(message='first')
    second = print_op(message='second').after(first)
    third = print_op(message='third').after(second)

This design produces the following result:
Kubeflow_Pipelines

load_component_from_text exit handler

import kfp.components as components
from kfp import dsl

exit_op = components.load_component_from_text("""
name: 'Print Message Component'
description: Prints a message.
inputs:
  - {name: message, type: String, description: 'Required. The message to print.'}
implementation:
  container:
    image: python:3.7-alpine
    command: [python]
    args: [
      -c,
      import sys; print(sys.argv.pop() or 'FALLBACK MESSAGE'),
      {inputValue: message},
    ]
""")

def print_message(message: str) -> None:
  print(message or 'NO MESSAGE SUPPLIED!')
print_op = components.func_to_container_op(print_message, base_image='python:3.7-alpine')

@dsl.pipeline(name='simple pipeline')
def pipeline_func():
  exit_handler = exit_op(message='EXITED')
  exit_handler.is_exit_handler=True
  with dsl.ExitHandler(exit_handler):
    first = print_op(message='first')
    second = print_op(message='second').after(first)
    third = print_op(message='third').after(second)

This design produces the following result:
Kubeflow_Pipelines

pure dsl.ContainerOp exit handler

import kfp.dsl as dsl
import kfp.components as components

def print_message(message: str) -> None:
  print(message or 'NO MESSAGE SUPPLIED!')
print_op = components.func_to_container_op(print_message, base_image='python:3.7-alpine')

@dsl.pipeline(name='simple pipeline')
def pipeline_func():
  exit_handler = dsl.ContainerOp(
    name='Exit Handler',
    is_exit_handler=True,
    image='python:3.7-alpine',
    command='python',
    arguments=[ 
      '-c',
      "print('EXITED')"
    ]
  )
  with dsl.ExitHandler(exit_handler):
    first = print_op(message='first')
    second = print_op(message='second').after(first)
    third = print_op(message='third').after(second)

This design correctly produces the following result:
Kubeflow_Pipelines

Anything else you would like to add:
Ideally this boolean value would be supplied with all calls that return a component factory function (to avoid needing to set it explicitly on the resulting operation).

@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 26, 2019

This is indeed an issue - is_exit_handler is only evaluated at construction time.
We should probably change the dsl.ExitHandler context manager so that it fixes the exit handler task (removes it from the op groups).

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 19, 2019

Fixed by #2411

@Ark-kun Ark-kun closed this as completed Oct 19, 2019
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this issue Oct 22, 2023
* Add phase 1 implementation of new storage spec

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* Storagespec refactoring

* Some refactoring

- Changed GetStorageURI func to always return the storageUri field and not impute from StorageSpec information
- Instead have the addStorageSpecAnnotations func set the storage uri annotation if a path is specified
- Deduplicated logic in predictor.go which sets the storage uri annotation from explicit storageUri field, and fail if the annotation is already set (i.e. if both storage.path and storageUri are specified)
- Renamed CreateStorageSpecSecretVolumeAndEnv func to CreateStorageSpecSecretEnvs and refactored the logic to fail if an explicit secret storageKey is not found, but not if it's a default one
- Renamed default storage keys to look up to "default" and "default_s3" depending on whether storage type information was provided
- Moved some string names to constants
- Removed now-not-applicable predictor_test.go and explainer_test.go
- Extended tests in storage_initializer_injector_test.go to cover StorageSpec cases

* Also check for existence of StorageSpec in IsMMSPredictor func

Co-authored-by: Nick Hill <nickhill@us.ibm.com>
Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* update python server with the new env variables

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* Update example due to the recent sklearn library upgrade

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* change static int to compute from variable length

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* abstract predictor and explainer validate function. Also add storageSpec validation

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* remove reduntant storageSpec check in the service_account_credentials.go

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* Add aws anonymous mode flog

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* generate python sdk and add storagespec s3 e2e test

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* update ci overlays

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* Proxy minio service to CI client.

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* move minio model init to a k8s job

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* deploy minio job with permitted ns

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* Add non-codegen import

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

* clean up new merges

Signed-off-by: tomcli <tommy.chaoping.li@ibm.com>

Co-authored-by: Nick Hill <nickhill@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants