[4/n] add python api for replacing local file references with source control links #21675

benpankow · 2024-05-06T23:22:15Z

Summary

Adds the link_to_source_control utility fn which converts all local source code reference metadata in the passed assets to references to source control. These local paths are mapped to the path in source control using a user-passed local git root. The path from the git root to the file locally is used as the filepath within source control.

For example:

@asset 
def my_asset() -> pd.DataFrame:
  ...

@asset 
def my_other_asset() -> pd.DataFrame:
  ...

defs = Definitions(
  assets=link_to_source_control(
    with_source_code_references([my_asset, my_other_asset]),
    source_control_url="https://github.com/dagster-io/dagster",
    source_control_branch="master",
    repository_root_absolute_path=file_relative_path(__file__, "../../"),
  )
)

A stacked PR will introduce a utility method that will wrap both link_to_source_control and with_source_code_references and will decide whether to link to source control based on whether the definitions are being loaded in a cloud context.

Test Plan

Unit tests.

benpankow · 2024-05-06T23:22:25Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @benpankow and the rest of your teammates on Graphite

github-actions · 2024-05-07T18:10:04Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-km41nn0l3-elementl.vercel.app
https://benpankow-first-pass-source-control-link.core-storybook.dagster-docs.io

Built with commit 52ed3c8.
This pull request is being automatically deployed with vercel-action

benpankow · 2024-05-07T18:30:13Z

python_modules/dagster/dagster/_core/definitions/metadata/source_code.py

+    source_control_branch: str,
+    repository_root_absolute_path: Union[Path, str],
+) -> Sequence[Union["AssetsDefinition", "SourceAsset", "CacheableAssetsDefinition"]]:
+    if "gitlab.com" in source_control_url:


I figure market share of these 2 is large enough for the fallback case not to matter too much, but worth flagging that we only explicitly can build code links for these right now

python_modules/dagster/dagster/_core/definitions/metadata/source_code.py

python_modules/dagster/dagster/_core/definitions/metadata/__init__.py

python_modules/dagster/dagster/_core/definitions/metadata/source_code.py

python_modules/dagster-graphql/dagster_graphql/implementation/events.py

q

rexledesma

Moving ts files to another PR would be nice for easier review.

Main question is whether we can infer some of these parameters on behalf of the user.

rexledesma · 2024-05-08T15:52:00Z

python_modules/dagster/dagster/_core/definitions/metadata/source_code.py

+
+@experimental
+@whitelist_for_serdes
+class CodeReferencesMetadataValue(DagsterModel, MetadataValue["CodeReferencesMetadataValue"]):


naming: I think we can drop the s and make CodeReferences into just CodeReference?

"Reference" is one of those nice irregular nouns that can describe a plurality in its singular form (e.g. a "reference" can hold many "references")

Suggested change

class CodeReferencesMetadataValue(DagsterModel, MetadataValue["CodeReferencesMetadataValue"]):

class CodeReferenceMetadataValue(DagsterModel, MetadataValue["CodeReferencesMetadataValue"]):

Also need to update the docstring.

I think this is showing as new because the stacked PRs were rebased - it's not new in this one - same w/ the .ts files unfortunately. Should be fixed now at least.

See this PR for brief naming discussion - personally I bias towards References because it makes clear that the value can hold more than one reference. When I see reference in the wild I typically interpret it as singular unless it's used as an adjective (e.g. reference material).

rexledesma · 2024-05-08T15:52:55Z

python_modules/dagster/dagster/_core/definitions/metadata/source_code.py

+        return self
+
+
+def local_source_path_from_fn(fn: Callable[..., Any]) -> Optional[LocalFileCodeReference]:


nit: use pathlib :)

rexledesma · 2024-05-08T16:00:36Z

python_modules/dagster/dagster/_core/definitions/metadata/source_code.py

+
+
+@experimental
+def link_to_source_control(


Could we potentially infer this information on behalf of the user by running a git command?

Yes, or just walking the folder structure (since in some cases the user may not install git in the target image, in the cloud context at least). Stacked PR will have a layered utility fn that will have default behavior.

sryza

The proliferation of these public helpers makes me a little nervous, especially because there's functionality that's specific to individual technologies. E.g. should this be part of dagster-github instead? I don't have a grand unifying philosophy of how they should be named which also makes me a little nervous.

So I think worth getting @schrockn 's thoughts too if he has bandwidth.

@benpankow thanks for bearing with the agonizing naming etc. process on these public APIs.

schrockn

@sryza thanks for flagging me on this one. I too share the concern about exposing all of these top-level exports. I didn't realize we were exposing them at the top-level. I'd rather not do that. I was assuming we were building these to get a heartbeat up and running for our own usage, rather than having them be publicly exposed APIs.

python_modules/dagster/dagster/__init__.py

schrockn · 2024-05-09T01:04:23Z

python_modules/dagster/dagster/__init__.py

    UrlMetadataValue as UrlMetadataValue,
+    link_to_source_control as link_to_source_control,
    with_source_code_references as with_source_code_references,


I didn't realize with_source_code_references was a top-level export. Has this shipped already?

No, it'll go out with next week's release, so we can definitely roll this back.

Will drop the top-level export for now.

schrockn · 2024-05-09T01:06:49Z

python_modules/dagster/dagster/_core/definitions/metadata/source_code.py

+def _convert_local_path_to_source_control_path_single_definition(
+    base_source_control_url: str,
+    repository_root_absolute_path: str,
+    assets_def: Union["AssetsDefinition", "SourceAsset", "CacheableAssetsDefinition"],
+) -> Union["AssetsDefinition", "SourceAsset", "CacheableAssetsDefinition"]:
+    from dagster._core.definitions.assets import AssetsDefinition
+
+    # SourceAsset doesn't have an op definition to point to - cacheable assets
+    # will be supported eventually but are a bit trickier
+    if not isinstance(assets_def, AssetsDefinition):
+        return assets_def
+
+    metadata_by_key = dict(assets_def.metadata_by_key) or {}


I believe I already tagged @smackesey in at some point where I saw similar code but it would be far preferable in my view to try to structure this to work on a canonicalized AssetsDefinition

github-actions · 2024-05-09T16:14:14Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-k1ozzca0o-elementl.vercel.app
https://benpankow-first-pass-source-control-link.dagster.dagster-docs.io

Direct link to changed pages:

queue

schrockn

Cool makes sense to me. Thanks for working through this!

sryza

lgtm as well!

benpankow · 2024-05-15T21:09:30Z

Merge activity

May 15, 2:09 PM PDT: @benpankow started a stack merge that includes this pull request via Graphite.
May 15, 2:11 PM PDT: Graphite rebased this pull request as part of a merge.
May 15, 2:14 PM PDT: @benpankow merged this pull request with Graphite.

…control links

@asset

…control links (dagster-io#21675) ## Summary Adds the `link_to_source_control` utility fn which converts all local source code reference metadata in the passed assets to references to source control. These local paths are mapped to the path in source control using a user-passed local git root. The path from the git root to the file locally is used as the filepath within source control. For example: ```python @asset def my_asset() -> pd.DataFrame: ... @asset def my_other_asset() -> pd.DataFrame: ... defs = Definitions( assets=link_to_source_control( with_source_code_references([my_asset, my_other_asset]), source_control_url="https://github.com/dagster-io/dagster", source_control_branch="master", repository_root_absolute_path=file_relative_path(__file__, "../../"), ) ) ``` A stacked PR will introduce a utility method that will wrap both `link_to_source_control` and `with_source_code_references` and will decide whether to link to source control based on whether the definitions are being loaded in a cloud context. ## Test Plan Unit tests.

This was referenced May 6, 2024

[3/n] display link to editor in the asset catalog #21468

Merged

[1/n] Add with_source_code_references fn to add source location metadata to assets #21465

Merged

[2/n] add frontend code link protocol customization in user settings #21467

Merged

benpankow mentioned this pull request May 6, 2024

separate out __init__ file for metadata pkg #21576

Merged

benpankow force-pushed the benpankow/add-code-source-link-frontend branch from e362410 to 978e409 Compare May 7, 2024 17:15

benpankow force-pushed the benpankow/first-pass-source-control-link branch from 8497f0b to 5e81cbf Compare May 7, 2024 17:15

benpankow force-pushed the benpankow/add-code-source-link-frontend branch from 978e409 to 7b3ff2a Compare May 7, 2024 17:48

benpankow force-pushed the benpankow/first-pass-source-control-link branch 2 times, most recently from 3e2b861 to 4e1ad1b Compare May 7, 2024 18:04

benpankow marked this pull request as ready for review May 7, 2024 18:27

benpankow requested review from sryza and rexledesma May 7, 2024 18:29

benpankow commented May 7, 2024

View reviewed changes

sryza previously requested changes May 7, 2024

View reviewed changes

benpankow requested a review from sryza May 7, 2024 23:23

benpankow force-pushed the benpankow/add-code-source-link-frontend branch from 243a068 to bee4264 Compare May 7, 2024 23:23

benpankow force-pushed the benpankow/first-pass-source-control-link branch from b5206d2 to 366b4d4 Compare May 7, 2024 23:23

benpankow mentioned this pull request May 8, 2024

[5/n] frontend for url code references #21713

Merged

benpankow force-pushed the benpankow/add-code-source-link-frontend branch from bee4264 to 85750bb Compare May 8, 2024 15:43

Base automatically changed from benpankow/add-code-source-link-frontend to master May 8, 2024 15:45

rexledesma reviewed May 8, 2024

View reviewed changes

benpankow force-pushed the benpankow/first-pass-source-control-link branch from 395cffb to df47725 Compare May 9, 2024 00:06

benpankow requested a review from rexledesma May 9, 2024 00:12

sryza reviewed May 9, 2024

View reviewed changes

schrockn previously requested changes May 9, 2024

View reviewed changes

benpankow force-pushed the benpankow/first-pass-source-control-link branch from df47725 to 52ed3c8 Compare May 9, 2024 16:10

benpankow requested review from sryza and schrockn May 14, 2024 16:19

benpankow requested a review from smackesey May 15, 2024 19:51

schrockn approved these changes May 15, 2024

View reviewed changes

sryza approved these changes May 15, 2024

View reviewed changes

benpankow force-pushed the benpankow/first-pass-source-control-link branch from 52ed3c8 to 70dd254 Compare May 15, 2024 20:42

benpankow mentioned this pull request May 15, 2024

export code reference classes at dagster package #21881

Open

benpankow added 7 commits May 15, 2024 21:10

[4/n] add python api for replacing local file references with source …

3f34c71

…control links

put off gql changes

dd354f0

gitlab

166ff8c

respond to review comments

9a0c469

rename

fa128e3

respond to some comments

01b7dc0

rm toplevel exports for now

395c0c6

benpankow force-pushed the benpankow/first-pass-source-control-link branch from 70dd254 to 395c0c6 Compare May 15, 2024 21:10

benpankow merged commit 52cc1b8 into master May 15, 2024
1 check was pending

benpankow deleted the benpankow/first-pass-source-control-link branch May 15, 2024 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4/n] add python api for replacing local file references with source control links #21675

[4/n] add python api for replacing local file references with source control links #21675

benpankow commented May 6, 2024 •

edited

Loading

benpankow commented May 6, 2024 •

edited

Loading

github-actions bot commented May 7, 2024 •

edited

Loading

benpankow May 7, 2024

rexledesma left a comment

rexledesma May 8, 2024

benpankow May 9, 2024 •

edited

Loading

rexledesma May 8, 2024

rexledesma May 8, 2024

benpankow May 9, 2024

sryza left a comment

schrockn left a comment

schrockn May 9, 2024

benpankow May 9, 2024

benpankow May 9, 2024

schrockn May 9, 2024

github-actions bot commented May 9, 2024

schrockn left a comment

sryza left a comment

benpankow commented May 15, 2024 •

edited

Loading

	class CodeReferencesMetadataValue(DagsterModel, MetadataValue["CodeReferencesMetadataValue"]):
	class CodeReferenceMetadataValue(DagsterModel, MetadataValue["CodeReferencesMetadataValue"]):

		return self


		def local_source_path_from_fn(fn: Callable[..., Any]) -> Optional[LocalFileCodeReference]:

[4/n] add python api for replacing local file references with source control links #21675

[4/n] add python api for replacing local file references with source control links #21675

Conversation

benpankow commented May 6, 2024 • edited Loading

Summary

Test Plan

benpankow commented May 6, 2024 • edited Loading

github-actions bot commented May 7, 2024 • edited Loading

Choose a reason for hiding this comment

rexledesma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpankow May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sryza left a comment

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 9, 2024

schrockn left a comment

Choose a reason for hiding this comment

sryza left a comment

Choose a reason for hiding this comment

benpankow commented May 15, 2024 • edited Loading

Merge activity

benpankow commented May 6, 2024 •

edited

Loading

benpankow commented May 6, 2024 •

edited

Loading

github-actions bot commented May 7, 2024 •

edited

Loading

benpankow May 9, 2024 •

edited

Loading

benpankow commented May 15, 2024 •

edited

Loading