-
Notifications
You must be signed in to change notification settings - Fork 791
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
Stacked @card v1.2 : Customizing @card
with current.card
#894
Conversation
- Added api for `current.cards` that handles multiple decorators -
…-custom-components
- 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
- tiny refactor at `CardDecorator._is_event_registered("step-init")`
- Added `current.cards.get(type='default')` method - All cards with `id` are accessible via `current.cards['xyz']` - `current.cards['xyz']` can also return non-editable cards - Added more comments and polished older comments - Added better warning messages.
- Shutting off `current.card[]` interface for invalid card ids .
…-custom-components-final-api-def
…-custom-components-final-api-def
- paring decospecs from `--with` doesn't conserve type. - Typecasting needs to be done in some form because of ambiguity of instantiation from cli and code.
…-custom-components-final-api-def
…-custom-components-final-api-def
…-custom-components-final-api-def
- stateful warnings all user facing interfaces.
- Adds warning once. - Option to disable it too.
@card
with current.card
@card
with current.card
@card
with current.card
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't look at everything in detail but hopefully some useful comments.
and re.match(CARD_ID_PATTERN, self.attributes["id"]) is None | ||
): | ||
# There should be a warning issued to the user that `id` doesn't match regex pattern | ||
# Since it is doesn't match pattern, we need to ensure that `id` is not accepted by `current` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiple typos in these two lines :)
@@ -63,10 +79,31 @@ def _walk(self, root): | |||
p = os.path.join(path, fname) | |||
yield p, p[prefixlen:] | |||
|
|||
def _is_event_registered(self, evt_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplify: return evt_name in self._called_once
…-custom-components-final-api-def
…-custom-components-final-api-def
…-custom-components-final-api-def
…-custom-components-final-api-def
…f' into mfcards-s1-current-card
- Renaming variables. - uuid to have a uuidv4 - simple typo fixes - step init sets card counts only once.
…f' into mfcards-s1-current-card
…f' into mfcards-s1-current-card
- changed default for `suppress_warnings` - Changed `"%s/%s"` pattern to os path join
…f' into mfcards-s1-current-card
…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 <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> 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>
* 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>
Stacked on : #893
Summary of Changes
This PR is an attempt to make
MetaflowCard
s customizable. Metaflow already ships withMetaflowCardComponent
s. These are isolated components for aMetaflowCard
. With this PR users will be able to dynamically add components in@step
code. This PR introduces thecurrent.card
interface to collectMetaflowCardComponent
s from userland (@step
code). This PR also introduces theid
argument in the@card
decorator and the--id
option in thecard create
command.Core Changes
current.card
to collect components from@step
codeid
property to@card
and cli.id
helps resolve custom components to the right card when using multiple decorators.Since there can be many cards, this PR introduces the concept of user-editable cards. This seemed a necessary bifurcation because decorators can be dynamically added at runtime. So to distinguish what cards users can editing we provide
MetaflowCard
s with aALLOW_USER_COMPONENTS
attribute.If
ALLOW_USER_COMPONENTS
is set toTrue
then theMetaflowCard
is eligible to have components added from the@step
code. We will call this an editable card.Once the card the editable, we can collect custom components from the step code in the following way :
Consider the above snippet. In the above scenario there are two
@card
decorators. One card is of typeblank
and another of typedefault
. Thecurrent.card
object helps collect "components" from@step
code to customize some card. In this case, it is aMarkdown
component.current.card
only accepts objects which are subclasses ofMetaflowCardComponent
. Since it is the responsibility ofcurrent.card
to resolve a component to the card mentioned in the decorator,current.card
first tries to resolve a default editable card. A default editable card is a card that will have access to thecurrent.card.append
/current.card.extend
methods. Since there can be many decorators and hence many cards,current.card.append
resolves this once the last@card
decorators calltask_pre_step
. In the above case when the user callsappend
, metaflow will only addMarkdown
to thedefault
card.current.card['a'].append
will add theMarkdown
to theblank
card. Herea
is anid
that can help resolve the component to the card. Since cards can be of many types, some cards can also not be user-editable (Cards withALLOW_USER_COMPONENTS=False
). Those cards won't be eligible to access thecurrent.card.append
current.card
(CardComponentCollector
)The
CardComponentCollector
is the object responsible for resolving aMetaflowCardComponent
to the card provided in the@card
decorator.Since there can be many cards,
CardComponentCollector
has a_finalize
function. The_finalize
function is called once the last@card
decorator callstask_pre_step
. The_finalize
function will try to find the default editable card from all the@card
decorators on the@step
. The default editable card is the card that can access thecurrent.card.append
/current.card.extend
methods. If there are multiple editable cards with noid
thencurrent.card
will throw warnings when users callcurrent.card.append
. This is done because we cannot resolve which card the component belongs.The
@card
decorator also exposes another argument calledcustomize=True
. Only one@card
decorator over a@step
can havecustomize=True
. Since cards can also be added from CLI when running a flow, adding@card(customize=True)
will set that particular card from the decorator as default editable. This means thatcurrent.card.append
will append to the card belonging to@card
withcustomize=True
. If there is more than one decorator withcustomize=True
we throw warnings thatcurrent.card.append
won't append to any card.One important feature of the
current.card
object is that it will not fail. Even when users try to accesscurrent.card.append
with multiple editable cards, we throw warnings but don't fail.current.card
will also not fail when a user tries to access a card of a non-existing id viacurrent.card['mycard']
. Sincecurrent.card['mycard']
gives reference to alist
ofMetaflowCardComponent
s, we return a non referencedlist
when users try to accesscurrent.card['my_non_existant_card']
TLDR
current.card.append
customizes the default editable card.ALLOW_USER_COMPONENTS=True
to be considered default editable.ALLOW_USER_COMPONENTS=False
are never default editable.id
argument to a card, in which case the card is editable throughcurrent.card[id].append
.current.card
.append calls show a warning but the call doesn’t fail.current.card['myid']
current.card.get(type=’pytorch’)
.Technical Details of Changes
Since the resolution of the default editable card is important for
current.card
, we need to ensure that we call the_finalize
function when the last@card
decorator callstask_pre_step
. To keep track of this the changes introduce class methods to set the total number of card decorators per@step
and increment a global step counter keeping a track of the number of decorators that have calledtask_pre_step
. Once the last@card
per@step
callstask_pre_step
we call the_finalize
function.The total number of card decorators per step is set in the
step_init
callback and the instantiation/addition of cards to theCardComponentCollector
happens in thetask_pre_step
. Once the user code finishes running we serialize theMetaflowCardComponent
s added tocurrent.card
.CardComponentCollector
has a_serialize
function which helpsserialize the components of the card to a JSON list or a list of strings. In the serialization process it calls the
MetaflowCardComponent.render()
function. TheMetaflowCardComponent.render()
is expected to return a JSON serializabledict
or string. This list is json dumped to atempfile
and passed to thecard create
subprocess.The
create
command in thecard_cli.py
reads the component file as-is and passes the serialized component list when instantiating the card class.