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

Refactor Variable CRUD methods in client #16564

Merged
merged 4 commits into from
Jan 1, 2025
Merged

Refactor Variable CRUD methods in client #16564

merged 4 commits into from
Jan 1, 2025

Conversation

aaazzam
Copy link
Collaborator

@aaazzam aaazzam commented Dec 31, 2024

Related to #16472, this PR continues the optimization pattern established for Artifacts, though with a smaller impact since Variables is one of our lighter primitives. The memory footprint reduction is negligible in this case, but the PR:

  • Refactors the client's Variable CRUD methods into a standalone module
  • Uses ForwardRefs for heavy imports, moving them into the call itself

While the memory impact is minimal for Variables specifically, applying this pattern consistently across all primitives will help us reach our optimization targets in #16472. The refactoring maintains full feature parity while improving the module's organization and type safety.

fun fact: our previous async client didn't have a create method, while our sync one didn't have a read_variables method

@github-actions github-actions bot added the bug Something isn't working label Dec 31, 2024
Copy link

codspeed-hq bot commented Dec 31, 2024

CodSpeed Performance Report

Merging #16564 will not alter performance

Comparing variables-client (e5e4944) with main (277731d)

Summary

✅ 3 untouched benchmarks

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.

One comment on using request from the base clients for type safety on the routes, but otherwise LGTM! I like how PrefectClient shrinks a little bit with each of these PRs.

Returns:
Information about the newly created variable.
"""
response = self._client.post(
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to use self.request for type safety here and in the other methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woof, yep!

@aaazzam aaazzam merged commit 82f35ec into main Jan 1, 2025
38 checks passed
@aaazzam aaazzam deleted the variables-client branch January 1, 2025 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants