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

feat: bulk save objects ignoring conflicts #185

Merged
merged 16 commits into from
Jul 17, 2024

Conversation

victorgarcia98
Copy link
Contributor

Previously, it wasn't possible to bulk save beliefs with the allow_overwrite = True. With the help of ON CONFLICT DO NOTHING, we can bulk save beliefs and ignore duplicated key conflicts.

@victorgarcia98 victorgarcia98 added the enhancement New feature or request label Jul 15, 2024
@victorgarcia98 victorgarcia98 self-assigned this Jul 15, 2024
Signed-off-by: Victor Garcia Reolid <[email protected]>
Copy link
Collaborator

@Flix6x Flix6x 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! I only have two minor requests, and it seems the pipeline still fails on pre-commit.

Was this option introduced recently in SQLAlchemy? If so, perhaps we should specify a minimum requirement, too.

timely_beliefs/beliefs/classes.py Outdated Show resolved Hide resolved
timely_beliefs/beliefs/classes.py Show resolved Hide resolved
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

One naming comment, one idea for complete support of allow_overwrite

timely_beliefs/beliefs/classes.py Outdated Show resolved Hide resolved
timely_beliefs/beliefs/classes.py Outdated Show resolved Hide resolved
Signed-off-by: Victor Garcia Reolid <[email protected]>
Signed-off-by: Victor Garcia Reolid <[email protected]>
Signed-off-by: Victor Garcia Reolid <[email protected]>
@victorgarcia98
Copy link
Contributor Author

victorgarcia98 commented Jul 15, 2024

Was this option introduced recently in SQLAlchemy? If so, perhaps we should specify a minimum requirement, too.

Good point. This feature was introduced in SQLAlchemy 1.1 and the project requires SQLAlchemy >= 2.0 so we don't need to constraint it.

Signed-off-by: Victor Garcia Reolid <[email protected]>
Copy link
Collaborator

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Great catch by @nhoening. By now I would prefer to use on_conflict_do_nothing over raising an exception, in case allow_overwrite is False.

And also I think we can now default to the faster bulk saving in the function signature. I think we used the slower option as a default because it was less prone to raise errors.

victorgarcia98 and others added 4 commits July 17, 2024 17:16
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: Victor Garcia Reolid <[email protected]>
Signed-off-by: Victor Garcia Reolid <[email protected]>
@victorgarcia98
Copy link
Contributor Author

Good idea! I've implemented the suggested changes and update the tests.

Signed-off-by: Victor Garcia Reolid <[email protected]>
@victorgarcia98 victorgarcia98 requested a review from Flix6x July 17, 2024 15:49
Copy link
Collaborator

@Flix6x Flix6x 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 addressing the failing tests.

Note that I had first resolved one of the failing tests differently. You revised add_to_session to make sure it does not fail if an empty frame is being saved, which was the right thing to do, while I first thought the test wasn't supposed to save an empty frame in the first place, so I had started revising the test and conftest. I reverted my changes, although there is still something odd going on there, namely this:

Due to the use of params=[1, 2] in the rolling_day_ahead_beliefs_about_time_slot_events fixture, each test that uses that fixture is being called twice as many times (in the case of test_adding_to_session, two sets of 4 times, due to the use of @pytest.mark.parametrize), where in the first round of calls, the fixture set up data from source A and none from B, and the second round the fixture set up data from source B and none from A. The test then selects only data from source B. All in all, the test setup and test cases could be clearer. But I'm leaving this for another day.

@Flix6x Flix6x merged commit a22cce8 into main Jul 17, 2024
5 checks passed
@Flix6x Flix6x deleted the feature/bulk-save-repeated-beliefs branch July 17, 2024 17:00
Flix6x added a commit that referenced this pull request Sep 2, 2024
Flix6x added a commit that referenced this pull request Sep 2, 2024
Signed-off-by: F.N. Claessen <[email protected]>
Flix6x added a commit that referenced this pull request Sep 2, 2024
* fix: raise IntegrityError (like we did before #185) instead of silently doing nothing

Signed-off-by: F.N. Claessen <[email protected]>

* revert: test updated from #185

Signed-off-by: F.N. Claessen <[email protected]>

---------

Signed-off-by: F.N. Claessen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants