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

Fix Run Meta Query Building #69

Merged
merged 9 commits into from
Mar 19, 2024
Merged

Fix Run Meta Query Building #69

merged 9 commits into from
Mar 19, 2024

Conversation

meksor
Copy link
Contributor

@meksor meksor commented Mar 18, 2024

Fixes query building for run meta entry queries. Also adds CI tests for postgres 15 and 16.
Throws a 400 when wrong parameters are used on the meta endpoint.
I also added a compatibility matrix to README.md.
Fixes #55.
(Maybe) implements PR#66?

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Thanks @meksor for digging through the code to fix this issue!

@@ -176,17 +174,3 @@ def unset_as_default_version(self, id: int) -> None:

run.is_default = False
self.session.commit()


def select_joined_run_index(repository, **kwargs) -> db.sql.Select:
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, I added this function here so that I could use it later to streamline the following almost-duplicate code parts:

runs["model"] = runs["model__id"].map(self.backend.models.map())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i think id take a look at this again in some sort of selection overhaul PR. The problem is that depending on what table is selected the joins have to be different (and there was a bug in the code applying filters twice) so i would try to find some new abstraction for this and the filters to make it less error prone in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also had to rename the columns selected ("model"->"model_name") because postgres 15 does not like columns in the result set with the same name as a table from the query

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Looks good to be merged and have the filters-signature discussion later…

@meksor meksor merged commit bca71e8 into main Mar 19, 2024
7 checks passed
@meksor meksor deleted the bugfix/meta-endpoint branch March 19, 2024 14:02
@glatterf42 glatterf42 mentioned this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP 500 on /v1/genie/meta/
2 participants