Use a separate cursor field when paginating through the asset catalog #23511
Conversation
gibsondan
changed the title
| # Filter out the initial location:repository from the id if present | ||
| # Assumes that '[' will never be in an asset key string after the initial character | ||
| cursor_string = cursor_string[cursor_string.rfind("[") :] |
There was a problem hiding this comment.
should we be more defensive here and check for str:str[ before mutating? just thinking about how this behaves if assumptions are invalid in the future
There was a problem hiding this comment.
the thing that I ran into here is that these are location and repository names and we don't seem to restrict "." in those names :/ (at least i can create a code location that starts with a "." locally)
There was a problem hiding this comment.
Similarly we appear to allow "[" in location names which is why I grabbed the last one
There was a problem hiding this comment.
Assumes that '[' will never be in an asset key string after the initial character
do we guard against this else where ? I was able to do
>>> from dagster import AssetKey
>>> AssetKey('foo[a]')
AssetKey(['foo[a]'])
There was a problem hiding this comment.
Yeah I tried to use it in an asset definition and got
File "/Users/dgibson/dagster/hi.py", line 4, in <module>
@asset(name="[hello")
^^^^^^^^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/decorators/asset_decorator.py", line 285, in inner
return create_assets_def_from_fn_and_decorator_args(args, fn)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/decorators/asset_decorator.py", line 482, in create_assets_def_from_fn_and_decorator_args
return builder.create_assets_definition()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/decorators/decorator_assets_definition_builder.py", line 550, in create_assets_definition
node_def=self.create_op_definition(),
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/decorators/decorator_assets_definition_builder.py", line 531, in create_op_definition
return _Op(
^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/decorators/op_decorator.py", line 125, in __call__
op_def = OpDefinition.dagster_internal_init(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/op_definition.py", line 206, in dagster_internal_init
return OpDefinition(
^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/decorator_utils.py", line 203, in wrapped_with_pre_call_fn
return fn(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/op_definition.py", line 183, in __init__
super(OpDefinition, self).__init__(
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/node_definition.py", line 54, in __init__
self._name = check_valid_name(name)
^^^^^^^^^^^^^^^^^^^^^^
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/utils.py", line 93, in check_valid_name
check_valid_chars(name)
File "/Users/dgibson/dagster/python_modules/dagster/dagster/_core/definitions/utils.py", line 101, in check_valid_chars
raise DagsterInvalidDefinitionError(
I should double check that we also don't let you materialize an asset with that key without it being a definition
There was a problem hiding this comment.
maybe something like https://regex101.com/r/RDXNfK/2 if we want to do regex approach
gibsondan
force-pushed
the
nodedefnamefix
branch
from
586c36f to
8f265e4
Compare
gibsondan
dismissed
prha’s stale review
take 2 - avoid IDs entirely and use a cursor
gibsondan
force-pushed
the
nodedefnamefix
branch
2 times, most recently
from
62985e3 to
377442a
Compare
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 175b69f. |
gibsondan
force-pushed
the
nodedefnamefix
branch
from
377442a to
9d3cdb1
Compare
gibsondan
changed the title
gibsondan
force-pushed
the
nodedefnamefix
branch
from
9d3cdb1 to
5851a45
Compare
Summary: The frontend passes in an ID which may have been emitted by get_unique_asset_id, which prepends a prefix before the path starts. Fix the asset key normalizer to account for that case. Test Plan: New test case hi
gibsondan
force-pushed
the
nodedefnamefix
branch
from
5851a45 to
175b69f
Compare
…#23511) Summary: The frontend passes in an ID which may have been emitted by get_unique_asset_id, which prepends a prefix before the path starts. Fix the asset key normalizer to account for that case. Test Plan: New test case ## Summary & Motivation ## How I Tested These Changes
Summary:
The frontend passes in an ID which may have been emitted by get_unique_asset_id, which prepends a prefix before the path starts. Fix the asset key normalizer to account for that case.
Test Plan: New test case
Summary & Motivation
How I Tested These Changes