Improve queries for pipelines, run templates, models and artifacts #3335
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| Returns: | ||
| The latest version for this artifact. | ||
| """ | ||
| if session := object_session(self): |
There was a problem hiding this comment.
I know it is convenient to use a function like this to fetch the session out of thin air, but this goes against some design principles and breaks the current order of things. For one, nobody would suspect that you have a dependency here on a DB session if they follow the code bread crumbs, this is completely hidden from a module dependency perspective.
I highly recommend that you used one of two alternative approaches, to make this dependency more obvious and to keep the design aligned to the current status quo:
First, make latest_version a function instead of a property and have it take in the DB session as an argument (let's call it get_latest_version instead). Then you have one of three choices:
- modify
to_modelto also take in the DB session as a mandatory argument, and make sure to pass it in all calls made toto_modelfrom the SQL Zen Store - keep the
to_modelsignature untouched, but callget_latest_versiondirectly from SQL Zen Store in addition toto_model. Or, to make it clear that this is mandatory, you could even addlatest_versionas a mandatory argument toto_modeland set it in SQL Zen Store to the result you get from calling theget_latest_versionfunction. - in addition to 2. above, move
get_latest_versionto SQL Zen Store, which is where it should belong to in the first place. The schema files shouldn't have any SQL statements in them. They're just there to define schemas after all. We have a method called_count_entitythat follows almost the same logic.
There was a problem hiding this comment.
Update: you explained that this is exactly how lazy loading works (hidden from the user and relies on the active DB session that created the schema object). In that case, this is fine. But please to check that:
- this is indeed tied to the object and represents the DB session that created it
- this is thread safe
There was a problem hiding this comment.
I verified that this is indeed always the session object that was used to create/read the schema, and also ran some stress tests on our server without issues.
Describe changes
As the runs and artifact/model versions can get quite a lot, this is very inefficient given that we're only interested in a single element (=the latest one). This PR fixes that by only querying for the latest run/version.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes