Page MenuHomeElementl

Limit the scope of hook context
ClosedPublic

Authored by cdecarolis on Thu, Apr 15, 5:34 PM.

Details

Summary

Hook context previously had a bunch of attributes that were not documented, and only used internally. This is due to it implementing an internal-use context, SystemContext. This further limits the scope by removing all of these attributes, and changing callsites accordingly.

Test Plan

unit and lint

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Apr 15, 6:01 PM
Harbormaster failed remote builds in B28934: Diff 35512!

Add back pipeline_name and run_id

want @yuhan to sign off on this one

as we are updating the logging_tags, can we test it in terminal and dagit to see if the hook events still behave the same

python_modules/dagster/dagster/core/events/__init__.py
1010–1014

lets use step_context then bc they are essentially the same thing.

python_modules/dagster/dagster/core/execution/context/system.py
536–537

should we still make this one public?

553

plz leave a comment explaining the case when a user calls hook_context.log.with_tags() as it mutates the logging_tags

554

i think we can eliminate this as well, bc it was only used when constructing the hook events, and now hook events depend on step_contetx. so we dont really need it here anymore

cdecarolis marked an inline comment as done.

Address comments

yuhan added inline comments.
python_modules/dagster/dagster_tests/core_tests/hook_tests/test_hook_run.py
61

nit: if we use solid_name instead (i.e. context.solid.name), we dont need to use the private attribute

This revision is now accepted and ready to land.Fri, Apr 16, 4:44 PM
python_modules/dagster/dagster_tests/core_tests/hook_tests/test_hook_run.py
61

in this case, I don't think we can use solid name, because the uniqueness of the name comes from the mapping to the outer composite

python_modules/dagster/dagster_tests/core_tests/hook_tests/test_hook_run.py
61

ah i see this will be inside a composite. nvm then. good to land