Stacked @card v1.2: Read cli changes for Supporting Multiple@cards
#895
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- modified datastore and fixed the exception class. - Added changes to cli
- Allowing some ambiguity in what pathspec argument. - `python myflow.py card view stepname` works now. - `python nonameflow.py card view runid/stepname/taskid` also works -
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
- added functionality to list cards about a task and even list it as JSON.
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
- instead of equating we check `startswith`
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
- listing all cards from the latest run when card list is called with no argument
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
- `--as-json` works for many cards and single cards
valayDave
changed the title
@cards@cards
This was referenced Jan 13, 2022
valayDave
commented
Jan 13, 2022
metaflow/plugins/cards/card_cli.py
Outdated
| def resolve_card( | ||
| ctx, | ||
| pathspec, | ||
| follow_resumed=True, | ||
| hash=None, | ||
| type=None, | ||
| card_id=None, | ||
| no_echo=False, | ||
| ): | ||
| """Resolves the card path based on the arguments provided. We allow identifier to be a pathspec or a id of card. |
There was a problem hiding this comment.
TODO : Need to remove the comment : We allow identifier to be a pathspec or a id of card.
valayDave
commented
Jan 13, 2022
metaflow/plugins/cards/card_cli.py
Outdated
| ctx.obj.echo(print_str, fg="green") | ||
| else: | ||
| card_pathspec = origin_taskpathspec | ||
| print_str = ("Resolving card resumed from: %s" % origin_taskpathspec,) |
There was a problem hiding this comment.
TODO : remove ,) in print_str
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
| if len(pthsplits) == 1: | ||
| # This means stepname | ||
| resolving_from = "stepname" | ||
| latest_run = Flow(flow_name).latest_run |
There was a problem hiding this comment.
Do we want the latest run or latest_successful_run?
There was a problem hiding this comment.
latest because we want to access cards during the execution of a flow. Because flows can have a long running time, it's much more convenient when accessing latest_run. Based on your comment I feel that adding namespace(None) at the start of the block was not the best idea as I should use namespace(None) when users add runid/stepname or runid/stepname/taskid
| if latest_run is not None: | ||
| run_id = latest_run.pathspec | ||
| try: | ||
| task = latest_run[pathspec].task |
There was a problem hiding this comment.
Do we want to warn if there are multiple tasks?
metaflow/plugins/cards/card_cli.py
Outdated
| task, | ||
| run_id, | ||
| resolved_from, | ||
| ) = resolve_task_from_pathspec(flow_name, pathspec) |
There was a problem hiding this comment.
run_id and resolved_from do not seem to be used except to raise an exception. The logic in resolve_task_from_pathspec is fairly long and could probably be simplified if the exception could be simplified a bit. Further, based on the logic in that function, there are cases where when task is none, run_id is also none and doesn't contain a lot of useful information. Perhaps we can simplify and just say that the string given as pathspec does not allow us to resolve to a task. You could, if you wanted to, also give a warning about resolving to multiple tasks.
There was a problem hiding this comment.
I can refactor the function by raising the error from inside resolve_task_from_pathspecand also returning only a pathspec.
| help="Print all available cards as a JSON object", | ||
| ) | ||
| @click.pass_context | ||
| def list( |
There was a problem hiding this comment.
It may make sense in these commands to respect the "--quiet" flag which should only print things that are easily machine parseable (at least that's the intent IIRC). This would impact the listing functions above to have a quiet mode in essence and maybe list things simply as opposed to prettily.
There was a problem hiding this comment.
Currently, we respect --quiet in list unless it is called with --as-json; the same goes for get. If I call card get stepname then it will print the card in the stdout. But calling card get stepname filename.html will save that card to the file. The test uses --quiet so I need this functionality to print/pass information when calling CLI commands in CliCheck.
|
|
||
| if task_card_path.is_file: | ||
| cards_found.append(card_path) | ||
|
|
||
| return cards_found | ||
|
|
||
| def create_full_path(self, card_path): |
There was a problem hiding this comment.
This may be a stupid comment so ignore it if it is but I thought we had a separate value for card root (METAFLOW_CARD_S3ROOT). Granted, it defaults to the one for the datastore but shouldn't we use the actual value in case it isn't like the one for the datastore?
There was a problem hiding this comment.
Given your reliance on the flow_datastore, it may be better to not leave this as an option to users (and maybe only allow them to configure the card sub-directory -- with restrictions that it should probably not be a valid python name to avoid conflicts with a flow).
There was a problem hiding this comment.
I actually set the storage root here. That's why the path generally lands up correct even when METAFLOW_CARD_S3ROOT is set.
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
- small refactor to cli + card ds.
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
…f' into multiple-card-decorators-read-cli
* 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 <rcledat@netflix.com> Co-authored-by: Brendan Gibson <brendan@outerbounds.co> Co-authored-by: Brendan Gibson <93726128+obgibson@users.noreply.github.com> Co-authored-by: adam <203779+seethroughdev@users.noreply.github.com> Co-authored-by: Romain Cledat <rcledat@netflix.com>
valayDave
added a commit
that referenced
this pull request
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 <brendan@outerbounds.co> Co-authored-by: Brendan Gibson <93726128+obgibson@users.noreply.github.com> Co-authored-by: adam <203779+seethroughdev@users.noreply.github.com> Co-authored-by: Romain Cledat <rcledat@netflix.com>
oavdeev
pushed a commit
that referenced
this pull request
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 <brendan@outerbounds.co> Co-authored-by: Brendan Gibson <93726128+obgibson@users.noreply.github.com> Co-authored-by: adam <203779+seethroughdev@users.noreply.github.com> Co-authored-by: Romain Cledat <rcledat@netflix.com> * 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 <brendan@outerbounds.co> Co-authored-by: Brendan Gibson <93726128+obgibson@users.noreply.github.com> Co-authored-by: adam <203779+seethroughdev@users.noreply.github.com> Co-authored-by: Romain Cledat <rcledat@netflix.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stacked on #894
Supported CLI Variations for
card get/card viewSupported for
card listWhy is this needed
Because we are now supporting multiple cards we need list functionality in the CLI to ensure that the test suite can work as it should. Since the
get/viewcommands are bound to resolve a single card, we need an unambiguous command that resolves all the metadata about all cards for a taskspec.Usage
TODO :
--idoption tocard view/card getget/viewsupported.get_cardsfunction.card listfunction