Skip to content
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

Allow blocks in deployment params #14741

Merged
merged 17 commits into from
Jul 29, 2024

Conversation

GalLadislav
Copy link
Contributor

@GalLadislav GalLadislav commented Jul 24, 2024

Context

Related #14393

I tried to "fix" Prefect's 3.0 inability to pass Blocks as deployments parameters. This feature is really important for me personally (and probably for others too). Since the related issue was closed as "not planned", i implemented something that could provide at least some functionality until it is properly addressed.

I would appreciate a feedback if this PR is useful and is worthwhile to polish it and add unittests etc. Thank you in advance for any guidance.

Implementation

Implementation is building on already existing UI features in FlowRunCreate for deployment runs that use passing {"$ref": "__uuid__"} as block reference. I have tracked to main issues which causes this feature to not work.

  1. Server's validation API does not allow this block definition,
  2. Block does not processes the $ref in init kwargs.

Server validation API

To allow $ref for blocks, preprocessing schemas modify block definition to allow either block definition or block reference.
Added two $ref formats, one that supports deployment flow run create form and one that supports multiple reference types.

Block initialization

For Block class i added method Block.load_from_ref that process either block document ID or supported reference dictionary format and returns loaded block with data from references block document.
Other changes:

  • created Block._load_from_block_document method to deal with duplicate code used both in Block.load and Block.load_from_ref.
  • added Block._get_block_document_by_id
  • Flow.validate_parameters updated to transform any block reference in given parameters to corresponding block with Block.load_from_ref.

Issues

  • API validation might check if blocks exist

Checklist

  • This pull request references any related issue by including "closes <link to issue>.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request adds functions or classes, it includes helpful docstrings.

@GalLadislav GalLadislav marked this pull request as ready for review July 24, 2024 16:32
@GalLadislav GalLadislav requested a review from cicdw as a code owner July 24, 2024 16:32
Copy link

codspeed-hq bot commented Jul 24, 2024

CodSpeed Performance Report

Merging #14741 will improve performances by 22%

Comparing GalLadislav:allow-blocks-in-deployment-params (0a12e51) with main (57f1cd4)

Summary

⚡ 1 improvements
✅ 4 untouched benchmarks

Benchmarks breakdown

Benchmark main GalLadislav:allow-blocks-in-deployment-params Change
bench_flow_call[options1] 294.1 ms 241 ms +22%

src/prefect/blocks/core.py Outdated Show resolved Hide resolved
@cicdw cicdw requested a review from desertaxle July 24, 2024 21:39
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few code nit picks - also could you add a test for what this initialization looks like, and also a test for using a block type hint with a flow that validates its parameters?

Ultimately it'd be better (and easier to reason about) if the API could pass the block slug to the client for validation, but this might be an OK patch in the meantime as that will involve more changes.

I've also tagged @desertaxle for review here, as he might have more insight into this implementation and whether there are any edge cases we should consider.

src/prefect/blocks/core.py Outdated Show resolved Hide resolved
src/prefect/blocks/core.py Outdated Show resolved Hide resolved
src/prefect/blocks/core.py Outdated Show resolved Hide resolved
@GalLadislav
Copy link
Contributor Author

I added block reference validation to check if it matches initialized block type. Not sure if both slug and type should be compared, or only one of them or should be validated differently.
Added also some tests to check basic cases. Probably more should be added. Also I was not sure if the location is ok.

@GalLadislav GalLadislav requested a review from cicdw July 26, 2024 02:02
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @GalLadislav, this will be a super useful feature! Let me know if you have any questions about any of my requested changes!

src/prefect/utilities/schema_tools/validation.py Outdated Show resolved Hide resolved
src/prefect/blocks/core.py Outdated Show resolved Hide resolved
@GalLadislav GalLadislav requested a review from desertaxle July 26, 2024 23:10
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty solid to me! I requested one additional test for nested block references, and will wait on @desertaxle's final review as well. Also note that you probably need to update with main and run pre-commit hooks for tests to pass 👍

Thank you for taking the time to work on this @GalLadislav !!

tests/blocks/test_block_reference.py Show resolved Hide resolved
@cicdw cicdw added the enhancement An improvement of an existing feature label Jul 27, 2024
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the awesome contribution @GalLadislav!

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again @GalLadislav for working with us on this and being so responsive to our feedback!!

@GalLadislav
Copy link
Contributor Author

Thank you @desertaxle @cicdw for your time and guidance. I am happy i could contribute.

@cicdw cicdw merged commit 544bd82 into PrefectHQ:main Jul 29, 2024
30 checks passed
@GalLadislav GalLadislav deleted the allow-blocks-in-deployment-params branch July 29, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants