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

Interrupting Workflow Does Not Terminate AWS Batch Job(s) #27

Closed
zcking opened this issue Dec 6, 2019 · 3 comments
Closed

Interrupting Workflow Does Not Terminate AWS Batch Job(s) #27

zcking opened this issue Dec 6, 2019 · 3 comments
Labels
help wanted Extra attention is needed

Comments

@zcking
Copy link

zcking commented Dec 6, 2019

Steps to Reproduce

  1. Clone the tutorials:
metaflow tutorials pull
cd metaflow-tutorials
  1. Configure AWS
metaflow configure aws
  1. Start Tutorial Episode 5: "Hello AWS"
python 05-helloaws/helloaws.py run
  1. As soon as you see the output "Task is starting (status STARTING)..." perform a keyboard interrupt (Ctrl+C) to stop the workflow. Note: because this hello AWS example runs so quickly, it may be easier if you add a time.sleep(10) and interrupt it during that delay

  2. View the AWS Batch Job console and notice the Job is not terminated

@savingoyal
Copy link
Collaborator

savingoyal commented Dec 6, 2019

Thanks for the bug report. Looks like a race condition. We do a best effort kill of Batch jobs. We will investigate.

@savingoyal savingoyal added the bug Something isn't working label Dec 6, 2019
@romain-intel
Copy link
Contributor

@zcking: I played around with this issue and found the following:

  • Batch does not immediately terminate; it may take 30s (or sometimes slightly more) for a termination API call to batch to be effective (in other words, MF does send termination commands to Batch jobs when you CTRL-C it but those termination commands do not have an immediate effect -- this is not something we have control over). Note that for very short jobs, this may manifest as the job being 'SUCCESSFUL' as opposed to 'FAILED' because the termination signal was handled after the job succeeded. If you add a time.sleep(60) for example, you are much more likely to see the job state in the AWS Batch Job console transition to 'FAILED' (after the aforementioned 30 seconds or so delay).
  • In some cases, the termination command is not sent from MF to Batch. This is a bug on our end and we will work on improving this. As @savingoyal mentioned, we do a best-effort kill of Batch jobs so we may still not be able to catch call cases but we will see if we can make it somewhat better. This is a fairly rare case so I suspect that what you are observing is more the delayed Batch termination issue listed above. You can change the sleep to 60 seconds for example to confirm -- if the jobs eventually transition to 'FAILED', it is the delayed batch issue, if not, it is this race (or some other issue that we haven't replicated yet); let me know what you observe and I can dig further if it is something I have not yet seen.

@zcking
Copy link
Author

zcking commented Dec 10, 2019

Thanks for the details @romain-intel I think that's a reasonable behavior, as is. I'm still very new to AWS Batch, and Metaflow even moreso, but I appreciate the willingness to improve for even these small details :)

@savingoyal savingoyal added help wanted Extra attention is needed and removed bug Something isn't working labels Dec 10, 2019
sappier pushed a commit to sappier/metaflow that referenced this issue May 31, 2021
valayDave added a commit that referenced this issue Jan 13, 2022
- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number
valayDave added a commit that referenced this issue Jan 24, 2022
* Multiple Cards Test Suite Mods (#27)

- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number

* Added test case for multiple cards

* Added tests card, summary :
- `current.cards['myid']` should be accessible when cards have an `id` argument in decorator
- `current.cards.append` should not work when there are no single default editable card.
- if a card has `ALLOW_USER_COMPONENTS=False` then it can still be edited via accessing it with `id` property.
- adding arbitrary information to `current.cards.append` should not break user code.
- Only cards with `ALLOW_USER_COMPONENTS=True` are considered default editable.
- If a single @card decorator is present with `id` then it `current.cards.append` should still work
- Access of `current.cards` with non existant id should not fail.
- `current.cards.append` should be accessible to the card with `customize=True`.
-

* fixed `DefaultEditableCardTest` test case
- Fixed comment
- Fixed the `customize=True` test case.

* ensure `test_pathspec_card` has no duplicates
- ensure entropy of rendered information is high enough to not overwrite a file.

* test case fix : `current.cards` to `current.card`

* Added Test case to support import of cards.
- Test case validates that broken card modules don't break metaflow
- test case validates that we are able to import cards from metaflow_extensions
- Test case validate that cards can be editable if they are importable.

* Added Env var to tests to avoid warnings added to cards.

* Added Test for card resume.

* Stacked @card v1.2: Card Dev Docs (#899)
valayDave added a commit that referenced this issue Jan 24, 2022
* setup import of cards from `metaflow_extensions` using `metaflow.extension_support`
- Added import modules in `card_modules`
- Added hook in card decorators to add custom packages

* Added some debug statements for external imports.

* Stacked @card v1.2 : Test cases for Multiple `@card`s (#898)

* Multiple Cards Test Suite Mods (#27)

- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number

* Added test case for multiple cards

* Added tests card, summary :
- `current.cards['myid']` should be accessible when cards have an `id` argument in decorator
- `current.cards.append` should not work when there are no single default editable card.
- if a card has `ALLOW_USER_COMPONENTS=False` then it can still be edited via accessing it with `id` property.
- adding arbitrary information to `current.cards.append` should not break user code.
- Only cards with `ALLOW_USER_COMPONENTS=True` are considered default editable.
- If a single @card decorator is present with `id` then it `current.cards.append` should still work
- Access of `current.cards` with non existant id should not fail.
- `current.cards.append` should be accessible to the card with `customize=True`.
-

* fixed `DefaultEditableCardTest` test case
- Fixed comment
- Fixed the `customize=True` test case.

* ensure `test_pathspec_card` has no duplicates
- ensure entropy of rendered information is high enough to not overwrite a file.

* test case fix : `current.cards` to `current.card`

* Added Test case to support import of cards.
- Test case validates that broken card modules don't break metaflow
- test case validates that we are able to import cards from metaflow_extensions
- Test case validate that cards can be editable if they are importable.

* Added Env var to tests to avoid warnings added to cards.

* Added Test for card resume.

* Stacked @card v1.2: Card Dev Docs (#899)

Co-authored-by: Romain Cledat <[email protected]>
valayDave added a commit that referenced this issue Jan 25, 2022
* accomodating changes from #833
- Minor tweaks to `graph.py`

* Stacked @card v1.2 : Namespace Packages with `@card` (#897)

* setup import of cards from `metaflow_extensions` using `metaflow.extension_support`
- Added import modules in `card_modules`
- Added hook in card decorators to add custom packages

* Added some debug statements for external imports.

* Stacked @card v1.2 : Test cases for Multiple `@card`s (#898)

* Multiple Cards Test Suite Mods (#27)

- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number

* Added test case for multiple cards

* Added tests card, summary :
- `current.cards['myid']` should be accessible when cards have an `id` argument in decorator
- `current.cards.append` should not work when there are no single default editable card.
- if a card has `ALLOW_USER_COMPONENTS=False` then it can still be edited via accessing it with `id` property.
- adding arbitrary information to `current.cards.append` should not break user code.
- Only cards with `ALLOW_USER_COMPONENTS=True` are considered default editable.
- If a single @card decorator is present with `id` then it `current.cards.append` should still work
- Access of `current.cards` with non existant id should not fail.
- `current.cards.append` should be accessible to the card with `customize=True`.
-

* fixed `DefaultEditableCardTest` test case
- Fixed comment
- Fixed the `customize=True` test case.

* ensure `test_pathspec_card` has no duplicates
- ensure entropy of rendered information is high enough to not overwrite a file.

* test case fix : `current.cards` to `current.card`

* Added Test case to support import of cards.
- Test case validates that broken card modules don't break metaflow
- test case validates that we are able to import cards from metaflow_extensions
- Test case validate that cards can be editable if they are importable.

* Added Env var to tests to avoid warnings added to cards.

* Added Test for card resume.

* Stacked @card v1.2: Card Dev Docs (#899)

Co-authored-by: Romain Cledat <[email protected]>
valayDave added a commit that referenced this issue Jan 25, 2022
* user-facing `MetaflowCardComponent`s
- import via `from metaflow.cards import Artifact,..`

* 7 Major Changes:
- Removed `Section` component from `metaflow.cards`
- Making user-facing component inherent to `UserComponent` which in turn inherits `MetaflowCardComponent`
- Wrap all `UserComponents` inside a section after rendering everything per card
- created a `render_safely` decorator to ensure fail-safe render of `UserComponent`s
- removed code from component serializer which used internal components
- Refactored some components that return render
- Added docstrings to all components.

* JS + CSS + Cards UI Build

* Stacked @card v1.2 : Graph Related Changes to card cli (#911)

* accomodating changes from #833
- Minor tweaks to `graph.py`

* Stacked @card v1.2 : Namespace Packages with `@card` (#897)

* setup import of cards from `metaflow_extensions` using `metaflow.extension_support`
- Added import modules in `card_modules`
- Added hook in card decorators to add custom packages

* Added some debug statements for external imports.

* Stacked @card v1.2 : Test cases for Multiple `@card`s (#898)

* Multiple Cards Test Suite Mods (#27)

- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number

* Added test case for multiple cards

* Added tests card, summary :
- `current.cards['myid']` should be accessible when cards have an `id` argument in decorator
- `current.cards.append` should not work when there are no single default editable card.
- if a card has `ALLOW_USER_COMPONENTS=False` then it can still be edited via accessing it with `id` property.
- adding arbitrary information to `current.cards.append` should not break user code.
- Only cards with `ALLOW_USER_COMPONENTS=True` are considered default editable.
- If a single @card decorator is present with `id` then it `current.cards.append` should still work
- Access of `current.cards` with non existant id should not fail.
- `current.cards.append` should be accessible to the card with `customize=True`.
-

* fixed `DefaultEditableCardTest` test case
- Fixed comment
- Fixed the `customize=True` test case.

* ensure `test_pathspec_card` has no duplicates
- ensure entropy of rendered information is high enough to not overwrite a file.

* test case fix : `current.cards` to `current.card`

* Added Test case to support import of cards.
- Test case validates that broken card modules don't break metaflow
- test case validates that we are able to import cards from metaflow_extensions
- Test case validate that cards can be editable if they are importable.

* Added Env var to tests to avoid warnings added to cards.

* Added Test for card resume.

* Stacked @card v1.2: Card Dev Docs (#899)

Co-authored-by: Romain Cledat <[email protected]>

Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: adam <[email protected]>
Co-authored-by: Romain Cledat <[email protected]>
valayDave added a commit that referenced this issue Jan 25, 2022
…895)

* Added the `--id` option to `card view`/`card get`
- modified datastore and fixed the exception class.
- Allowing some ambiguity in pathspec argument for `get/view` command.

* Added `id` argument to `get_cards`

* Bringing `card list` cli functionality from test-suite branch
- added functionality to list cards about a task and even list it as JSON.

* changed hash checking logic.
- instead of equating we check `startswith`

* Added list many feature for `card list`
- listing all cards from the latest run when card list is called with no argument

* Added `card list` print formatting changes
- `--as-json` works for many cards and single cards


* Stacked @card v1.2 : New `MetaflowCardComponent`s  (#896)

* user-facing `MetaflowCardComponent`s
- import via `from metaflow.cards import Artifact,..`

* 7 Major Changes:
- Removed `Section` component from `metaflow.cards`
- Making user-facing component inherent to `UserComponent` which in turn inherits `MetaflowCardComponent`
- Wrap all `UserComponents` inside a section after rendering everything per card
- created a `render_safely` decorator to ensure fail-safe render of `UserComponent`s
- removed code from component serializer which used internal components
- Refactored some components that return render
- Added docstrings to all components.

* JS + CSS + Cards UI Build

* Stacked @card v1.2 : Graph Related Changes to card cli (#911)

* accomodating changes from #833
- Minor tweaks to `graph.py`

* Stacked @card v1.2 : Namespace Packages with `@card` (#897)

* setup import of cards from `metaflow_extensions` using `metaflow.extension_support`
- Added import modules in `card_modules`
- Added hook in card decorators to add custom packages

* Added some debug statements for external imports.

* Stacked @card v1.2 : Test cases for Multiple `@card`s (#898)

* Multiple Cards Test Suite Mods (#27)

- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number

* Added test case for multiple cards

* Added tests card, summary :
- `current.cards['myid']` should be accessible when cards have an `id` argument in decorator
- `current.cards.append` should not work when there are no single default editable card.
- if a card has `ALLOW_USER_COMPONENTS=False` then it can still be edited via accessing it with `id` property.
- adding arbitrary information to `current.cards.append` should not break user code.
- Only cards with `ALLOW_USER_COMPONENTS=True` are considered default editable.
- If a single @card decorator is present with `id` then it `current.cards.append` should still work
- Access of `current.cards` with non existant id should not fail.
- `current.cards.append` should be accessible to the card with `customize=True`.
-

* fixed `DefaultEditableCardTest` test case
- Fixed comment
- Fixed the `customize=True` test case.

* ensure `test_pathspec_card` has no duplicates
- ensure entropy of rendered information is high enough to not overwrite a file.

* test case fix : `current.cards` to `current.card`

* Added Test case to support import of cards.
- Test case validates that broken card modules don't break metaflow
- test case validates that we are able to import cards from metaflow_extensions
- Test case validate that cards can be editable if they are importable.

* Added Env var to tests to avoid warnings added to cards.

* Added Test for card resume.

* Stacked @card v1.2: Card Dev Docs (#899)

Co-authored-by: Romain Cledat <[email protected]>

Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: adam <[email protected]>
Co-authored-by: Romain Cledat <[email protected]>

Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: adam <[email protected]>
Co-authored-by: Romain Cledat <[email protected]>
valayDave added a commit that referenced this issue Jan 25, 2022
* allow passing userland `MetaflowCardComponents`
-  Added api for `current.cards` that handles multiple decorators

* `current.cards` with `id` support.
- Introduced `ALLOW_USER_COMPONENTS` attribute to `MetaflowCard` class
- setting `ALLOW_USER_COMPONENTS=True` allow editable cards
- Modified logic of `CardComponentCollector`
- `CardComponentCollector.append` only accessible to default editble card

* Added `customize=True` in `@card`
- `customize=True` only allowed for one @card deco per @step
- `customize=True` sets the card to be default editable
- `current.card.append` appends to @card with `customize=True`


* Stacked @card v1.2: Read cli changes for Supporting Multiple`@card`s (#895)

* Added the `--id` option to `card view`/`card get`
- modified datastore and fixed the exception class.
- Allowing some ambiguity in pathspec argument for `get/view` command.

* Added `id` argument to `get_cards`

* Bringing `card list` cli functionality from test-suite branch
- added functionality to list cards about a task and even list it as JSON.

* changed hash checking logic.
- instead of equating we check `startswith`

* Added list many feature for `card list`
- listing all cards from the latest run when card list is called with no argument

* Added `card list` print formatting changes
- `--as-json` works for many cards and single cards


* Stacked @card v1.2 : New `MetaflowCardComponent`s  (#896)

* user-facing `MetaflowCardComponent`s
- import via `from metaflow.cards import Artifact,..`

* 7 Major Changes:
- Removed `Section` component from `metaflow.cards`
- Making user-facing component inherent to `UserComponent` which in turn inherits `MetaflowCardComponent`
- Wrap all `UserComponents` inside a section after rendering everything per card
- created a `render_safely` decorator to ensure fail-safe render of `UserComponent`s
- removed code from component serializer which used internal components
- Refactored some components that return render
- Added docstrings to all components.

* JS + CSS + Cards UI Build

* Stacked @card v1.2 : Graph Related Changes to card cli (#911)

* accomodating changes from #833
- Minor tweaks to `graph.py`

* Stacked @card v1.2 : Namespace Packages with `@card` (#897)

* setup import of cards from `metaflow_extensions` using `metaflow.extension_support`
- Added import modules in `card_modules`
- Added hook in card decorators to add custom packages

* Added some debug statements for external imports.

* Stacked @card v1.2 : Test cases for Multiple `@card`s (#898)

* Multiple Cards Test Suite Mods (#27)

- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number

* Added test case for multiple cards

* Added tests card, summary :
- `current.cards['myid']` should be accessible when cards have an `id` argument in decorator
- `current.cards.append` should not work when there are no single default editable card.
- if a card has `ALLOW_USER_COMPONENTS=False` then it can still be edited via accessing it with `id` property.
- adding arbitrary information to `current.cards.append` should not break user code.
- Only cards with `ALLOW_USER_COMPONENTS=True` are considered default editable.
- If a single @card decorator is present with `id` then it `current.cards.append` should still work
- Access of `current.cards` with non existant id should not fail.
- `current.cards.append` should be accessible to the card with `customize=True`.
-

* fixed `DefaultEditableCardTest` test case
- Fixed comment
- Fixed the `customize=True` test case.

* ensure `test_pathspec_card` has no duplicates
- ensure entropy of rendered information is high enough to not overwrite a file.

* test case fix : `current.cards` to `current.card`

* Added Test case to support import of cards.
- Test case validates that broken card modules don't break metaflow
- test case validates that we are able to import cards from metaflow_extensions
- Test case validate that cards can be editable if they are importable.

* Added Env var to tests to avoid warnings added to cards.

* Added Test for card resume.

* Stacked @card v1.2: Card Dev Docs (#899)

Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: adam <[email protected]>
Co-authored-by: Romain Cledat <[email protected]>
oavdeev pushed a commit that referenced this issue Jan 25, 2022
* multiple decorator support

* Fixing comments

* comment fix.

* commet fix

* allow multiple decorators of same type from cli

* Stacked @card v1.2 : Customizing `@card` with `current.card` (#894)

* allow passing userland `MetaflowCardComponents`
-  Added api for `current.cards` that handles multiple decorators

* `current.cards` with `id` support.
- Introduced `ALLOW_USER_COMPONENTS` attribute to `MetaflowCard` class
- setting `ALLOW_USER_COMPONENTS=True` allow editable cards
- Modified logic of `CardComponentCollector`
- `CardComponentCollector.append` only accessible to default editble card

* Added `customize=True` in `@card`
- `customize=True` only allowed for one @card deco per @step
- `customize=True` sets the card to be default editable
- `current.card.append` appends to @card with `customize=True`


* Stacked @card v1.2: Read cli changes for Supporting Multiple`@card`s (#895)

* Added the `--id` option to `card view`/`card get`
- modified datastore and fixed the exception class.
- Allowing some ambiguity in pathspec argument for `get/view` command.

* Added `id` argument to `get_cards`

* Bringing `card list` cli functionality from test-suite branch
- added functionality to list cards about a task and even list it as JSON.

* changed hash checking logic.
- instead of equating we check `startswith`

* Added list many feature for `card list`
- listing all cards from the latest run when card list is called with no argument

* Added `card list` print formatting changes
- `--as-json` works for many cards and single cards


* Stacked @card v1.2 : New `MetaflowCardComponent`s  (#896)

* user-facing `MetaflowCardComponent`s
- import via `from metaflow.cards import Artifact,..`

* 7 Major Changes:
- Removed `Section` component from `metaflow.cards`
- Making user-facing component inherent to `UserComponent` which in turn inherits `MetaflowCardComponent`
- Wrap all `UserComponents` inside a section after rendering everything per card
- created a `render_safely` decorator to ensure fail-safe render of `UserComponent`s
- removed code from component serializer which used internal components
- Refactored some components that return render
- Added docstrings to all components.

* JS + CSS + Cards UI Build

* Stacked @card v1.2 : Graph Related Changes to card cli (#911)

* accomodating changes from #833
- Minor tweaks to `graph.py`

* Stacked @card v1.2 : Namespace Packages with `@card` (#897)

* setup import of cards from `metaflow_extensions` using `metaflow.extension_support`
- Added import modules in `card_modules`
- Added hook in card decorators to add custom packages

* Added some debug statements for external imports.

* Stacked @card v1.2 : Test cases for Multiple `@card`s (#898)

* Multiple Cards Test Suite Mods (#27)

- Added `list_cards` to `CliCheck` and `MetadataCheck`
- Bringing #875 into the code
- Added a card that prints taskspec with random number

* Added test case for multiple cards

* Added tests card, summary :
- `current.cards['myid']` should be accessible when cards have an `id` argument in decorator
- `current.cards.append` should not work when there are no single default editable card.
- if a card has `ALLOW_USER_COMPONENTS=False` then it can still be edited via accessing it with `id` property.
- adding arbitrary information to `current.cards.append` should not break user code.
- Only cards with `ALLOW_USER_COMPONENTS=True` are considered default editable.
- If a single @card decorator is present with `id` then it `current.cards.append` should still work
- Access of `current.cards` with non existant id should not fail.
- `current.cards.append` should be accessible to the card with `customize=True`.
-

* fixed `DefaultEditableCardTest` test case
- Fixed comment
- Fixed the `customize=True` test case.

* ensure `test_pathspec_card` has no duplicates
- ensure entropy of rendered information is high enough to not overwrite a file.

* test case fix : `current.cards` to `current.card`

* Added Test case to support import of cards.
- Test case validates that broken card modules don't break metaflow
- test case validates that we are able to import cards from metaflow_extensions
- Test case validate that cards can be editable if they are importable.

* Added Env var to tests to avoid warnings added to cards.

* Added Test for card resume.

* Stacked @card v1.2: Card Dev Docs (#899)

Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: adam <[email protected]>
Co-authored-by: Romain Cledat <[email protected]>

* Nit fix in error.

* Fixing bug in decorator

* whitespace.

* Changing import scheme of warning variable.

* fixing warning suppression env var setting

Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: Brendan Gibson <[email protected]>
Co-authored-by: adam <[email protected]>
Co-authored-by: Romain Cledat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants