Filter out Copr builds without SRPM in SQL #2863
Conversation
Summary of ChangesHello @m-blaha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the data retrieval process for Copr build lists by migrating the filtering of incomplete builds from application-level Python code to the database query. This enhancement ensures that the API consistently delivers the expected number of results and improves overall performance by optimizing data processing at its source. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
m-blaha
force-pushed
the
filter-out-no-source
branch
from
ab917f1 to
fe6327e
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly moves the filtering of Copr builds from Python code into the SQL query. This is a good change that resolves an issue with pagination and improves efficiency. The implementation is clean and the reasoning is sound. I have one suggestion for a further performance improvement to address a potential N+1 query problem in the API endpoint, which would make this part of the code even more efficient.
| for build in CoprBuildTargetModel.get_merged_chroots(first, last): | ||
| build_info = CoprBuildTargetModel.get_by_build_id(build.build_id, None) | ||
| if build_info.status == BuildStatus.waiting_for_srpm: | ||
| continue | ||
| if ( | ||
| build_info.status == BuildStatus.failure | ||
| and not build_info.build_start_time | ||
| and not build_info.build_logs_url | ||
| ): | ||
| # SRPM build failed, it doesn't make sense to list this build | ||
| continue | ||
| project_info = build_info.get_project() |
There was a problem hiding this comment.
This loop currently causes an N+1 query problem. For each build returned by get_merged_chroots, you're making at least two more database queries: get_by_build_id and then get_project (which itself can trigger multiple lazy-loads). This can lead to significant performance degradation, especially with a large number of builds.
To resolve this, I recommend modifying CoprBuildTargetModel.get_merged_chroots to fetch all the necessary information in a single query by using joins and returning all required fields. This would eliminate the need for extra queries inside the loop.
For example, you could extend the query in get_merged_chroots to join with CoprBuildGroupModel, PipelineModel, ProjectEventModel, and GitProjectModel to retrieve fields like project_name, build_submitted_time, web_url, commit_sha, and project details. You would need to use an aggregate function (like min or max) on these additional fields within the group_by clause, since they will be the same for all chroots of a given build.
This would make the API endpoint much more performant.
There was a problem hiding this comment.
This makes sense, but is out of scope here. If we wanted to optimize the code, it should be tracked as a separate issue.
| .filter( | ||
| # Exclude builds without build_id - these are builds waiting for SRPM | ||
| # or where SRPM build failed, so technically they are not actual Copr | ||
| # builds yet. | ||
| CoprBuildTargetModel.build_id.isnot(None), | ||
| ) |
There was a problem hiding this comment.
unfortunately, I don't think this will solve the issue, the model has already set the build_id even when waiting for SRPM, example API response:
"anitya_package": null,
"anitya_project_id": null,
"anitya_project_name": null,
"anitya_version": null,
"branch_name": null,
"build_finished_time": null,
"build_id": "9923561",
"build_logs_url": null,
"build_start_time": null,
"build_submitted_time": 1765975741,
"built_packages": null,
"chroot": "rhel-9-x86_64",
"commit_sha": "93e63e21c0401256021ec3a0460a9517d4986267",
"copr_owner": "packit",
"copr_project": "theforeman-foreman-10802",
"issue_id": null,
"non_git_upstream": false,
"pr_id": 10802,
"project_url": "https://github.com/theforeman/foreman",
"release": null,
"repo_name": "foreman",
"repo_namespace": "theforeman",
"run_ids": [
1086847
],
"srpm_build_id": 520981,
"status": "waiting_for_srpm",
"web_url": "https://copr.fedorainfracloud.org/coprs/build/9923561/"
}
we are setting if here right after the build is submitted
There was a problem hiding this comment.
Yeah, you're right :( I'll try to find another way to filter builds directly in the query so I can keep removal of the Python-level filtering.
There was a problem hiding this comment.
I tried to replicate the additional Python filtering directly in the query.
m-blaha
force-pushed
the
filter-out-no-source
branch
from
fe6327e to
f7bbf3f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement. It correctly moves the filtering logic for Copr builds from the application layer to the database query. This change effectively resolves the described pagination issue and improves performance by filtering data at the source. The implementation is clean and the logic for excluding certain builds is correctly translated to SQLAlchemy. I have a couple of suggestions to enhance the readability of the new filter condition, but overall, this is a solid contribution.
| desc, | ||
| func, | ||
| null, | ||
| or_, |
There was a problem hiding this comment.
To improve the readability of the filter condition being added, consider importing and_ and not_ instead of or_. This will allow expressing the filter logic in a way that more directly mirrors the case being excluded, making the code easier to understand.
| or_, | |
| and_, | |
| not_, |
| or_( | ||
| CoprBuildTargetModel.status != BuildStatus.failure, | ||
| CoprBuildTargetModel.build_start_time.isnot(None), | ||
| CoprBuildTargetModel.build_logs_url.isnot(None), | ||
| ), |
There was a problem hiding this comment.
While this or_ condition is logically correct, it represents the inverted logic of what is being excluded. Using not_(and_(...)) would be more readable as it directly translates the condition for an SRPM build failure that should be excluded: status == failure AND build_start_time IS NULL AND build_logs_url IS NULL. This makes the intent of the filter clearer and closer to the original Python logic that was removed.
| or_( | |
| CoprBuildTargetModel.status != BuildStatus.failure, | |
| CoprBuildTargetModel.build_start_time.isnot(None), | |
| CoprBuildTargetModel.build_logs_url.isnot(None), | |
| ), | |
| not_( | |
| and_( | |
| CoprBuildTargetModel.status == BuildStatus.failure, | |
| CoprBuildTargetModel.build_start_time.is_(None), | |
| CoprBuildTargetModel.build_logs_url.is_(None), | |
| ) | |
| ), |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
m-blaha
force-pushed
the
filter-out-no-source
branch
from
f7bbf3f to
61f869f
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 45s |
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
this approach looks good to me! Do you think we could add a testcase in https://github.com/packit/packit-service/blob/main/tests_openshift/database/test_models.py and/or https://github.com/packit/packit-service/blob/main/tests_openshift/service/test_api.py? These tests are not run in CI unfortunately (yet), but are good for this kind of changes; they require compose setup, there is this Make target.
|
Sure, will try! |
m-blaha
force-pushed
the
filter-out-no-source
branch
from
61f869f to
e7fc6d9
Compare
|
@lbarcziova I've added the test |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 44s |
| or_( | ||
| CoprBuildTargetModel.status != BuildStatus.failure, | ||
| CoprBuildTargetModel.build_start_time.isnot(None), | ||
| CoprBuildTargetModel.build_logs_url.isnot(None), | ||
| ), |
There was a problem hiding this comment.
I checked now how we actually handle SRPM failure, and here it looks like we don't touch the Copr build models at all, we only change the status to cancelled, if the build is retriggered (by push or similar)
have you found some code that leaves the DB items in this state, or we could remove this and stick to only filtering based on status (probably add also BuildStatus.canceled)
There was a problem hiding this comment.
To be honest, I only reproduced current python filtering on database level. Let me check whether start_time/logs_url check is actually useful.
There was a problem hiding this comment.
@lbarcziova I checked the production database and found many copr_build_targets records missing start_time and build_logs_url and marked as failure. They are coming at a rate of approximately 80 records per day. It appears there was no copr.build.Start event, only copr.build.End for these builds (I checked the logs). I'm not sure whether these events were lost or never issued from the copr side. Some of these builds correspond to srpm build failures (e.g. https://copr.fedorainfracloud.org/coprs/packit/osbuild-image-builder-frontend-3989/build/10010130/), but others are regular target build failures where the srpm succeeded (e.g. https://copr.fedorainfracloud.org/coprs/packit/systemd-systemd-40311/build/10010141/). Is this a known issue, or should I file a report?
There was a problem hiding this comment.
FWIW, here is PR that introduced start_time/logs_url check - #2396 . It seems that the reason was to filter out log links with empty URLs.
There was a problem hiding this comment.
@m-blaha thanks a lot for checking this thoroughly. So there are records of Copr builds that ended with failure but still don't have the start_time and build_logs_url? That seems like a bug indeed, please do. But we can keep it in the PR like this as we already did the same filtering on the API level.
There was a problem hiding this comment.
I looked also into copr build logs and it seems to be an issue on the copr side. Filed an issue there - fedora-copr/copr#4127
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 46s |
|
Pull request merge failed: Required status check "Notes are either written, or there are none / release-notes" is expected. |
In addition to pagination slicing, the CoprBuildsList class uses Python code to filter out Copr builds that are waiting for an SRPM or whose SRPM build failed. This causes that the API in some cases can return fewer items than the user requested. Moving the filter into SQL resolves the problem. Resolves: packit#2505
m-blaha
force-pushed
the
filter-out-no-source
branch
from
e7fc6d9 to
e81528a
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 45s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 44s |
github-project-automation
bot
moved this from In review
to Done
in Packit pull requests
2 participants
In addition to pagination slicing, the CoprBuildsList class uses Python code to filter out Copr builds that are waiting for an SRPM or whose SRPM build failed. This causes that the API in some cases can return fewer items than the user requested.
Moving the filter into SQL resolves the problem. The SQL filter relies on the fact that the build_id field is NULL until the build is actually created by submitting to Copr.
Resolves: #2505