feat: Add matching sorts #497
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bgins
force-pushed
the
bgins/feat-add-matching-sorts
branch
16 times, most recently
from
9d9285f to
1985538
Compare
bgins
force-pushed
the
bgins/feat-add-matching-sorts
branch
3 times, most recently
from
97b21b9 to
836ecc7
Compare
narbs91
reviewed
Jan 29, 2025
pkg/solver/matcher/matcher.go
Outdated
Comment on lines
173
to
191
| span.AddEvent("get_deal.done", trace.WithAttributes(attribute.String("deal.id", deal.ID))) | ||
|
|
||
| // Add match decisions for all matching offers | ||
| for _, matchingOffer := range matchingResourceOffers { | ||
| addDealID := "" | ||
| if selectedResourceOffer.ID == matchingOffer.ID { | ||
| addDealID = deal.ID | ||
| } | ||
|
|
||
| // All match decisions had matching resource offers, set the result to true. | ||
| // The match decision has a deal ID if it's resource offer was selected. | ||
| span.AddEvent("add_match_decision.start") | ||
| _, err := db.AddMatchDecision(matchingOffer.ID, jobOffer.ID, addDealID, true) | ||
| if err != nil { | ||
| span.SetStatus(codes.Error, "unable to add match decision") | ||
| span.RecordError(err) | ||
| return nil, err | ||
| } | ||
| span.AddEvent("add_match_decision.done") |
There was a problem hiding this comment.
Maybe have this as it's own private function to shrink the size of the main function and help readability?
There was a problem hiding this comment.
Also inside the loop we want to have a log when a match is made (though this might flood the log file) or have a log at the end of the loop to signify how many matches were made
There was a problem hiding this comment.
Refactored this loop into a addMatchDecisions helper function: 5e99321
There was a problem hiding this comment.
And added the logging here: 2a57b7b
bgins
force-pushed
the
bgins/feat-add-matching-sorts
branch
from
c0ce9b4 to
c56ccfb
Compare
narbs91
approved these changes
Jan 31, 2025
We will re-use these for benchmarks in a new test file.
Our logMatch function expects a match result value. In addition, there's no advantage to using references here.
We will use these helpers for top-level solver integration tests in addition to the store integration tests.
We integrate at the solver to test matcher and store functionality together.
bgins
force-pushed
the
bgins/feat-add-matching-sorts
branch
from
2a57b7b to
ccd3946
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pull request makes the following changes:
GetJobOffersstore method with oldest first sort queryGetResourceOffersstore method with oldest first queryisCheaperOrOldermatch helperTestIsCheaperOrOlderunit testmatchOffersbenchmarkbenchGitHub actionIn addition, it makes supporting and refactoring changes:
matcher_test.gotest cases topkg/solver/matcher/testing.gofor reuse by the benchmarkstore_test.gostore helpers topkg/solver/testing.gofor reuse by the matching and sorting integration testmatchResult(fix forlogMatchlogger)This PR updates the matching algorithm with pricing and age sorting preferences. In addition, we add new test coverage, including relative benchmarks to report performance and alert on performance degradation.
Task/Issue reference
Closes: #492
Test plan
Unit tests
We have added a
TestIsCheaperOrOlderunit test to test ourisCheaperOrOldermatch helper. The test runs with the other unit tests or alone with:Integration tests
We have extended our store integration tests to cover our get oldest offer queries. Run the store integration tests:
The new tests are
TestJobOfferSortandTestResourceOfferSort.We have added a new matching and sorting integration test to the solver. Run the test with:
Run all of the integration tests with:
Benchmarks
Our first benchmark has arrived! We have added documentation describing the benchmark in the local development guide and a note celebrating them in our README. 🎉
The benchmark measures the performance of
matchOffers. Run it with:Our primary goal is relative benchmarking in CI. Aside from running in a isolated GitHub action, we make no attempts to control the environment where the benchmark runs. We want to know if a change has degraded performance, but we are not too concerned with accuracy.
The new
benchGitHub action runs the benchmark. We get some nice reporting and alerting frombenchmark-action/github-action-benchmark:benchaction. For example: https://github.com/Lilypad-Tech/lilypad/actions/runs/13023788174/attempts/1#summary-36329383772In addition, when we push to
main, the action publishes the benchmark results to a historical chart: https://lilypad-tech.github.io/lilypad/dev/bench/ (some sample data from commits on this PR, can clean up later)Details
The pricing and age sorting preferences use the following criteria:
In practice, we request job offers and resource offers from the database sorted oldest first. For each job offer, we build a list of matching resource offers. We apply the pricing preference over the matched resource offers:
We may be able to optimize in the future by retrieving resource offers cheapest first from the database, but we believe this to be a good starting point with room to grow. 😄
Establishing offer age
When we query the database store, we sort oldest first relying on the database engine to sort for us. We don't have this option for the memory store, so we use the job offer and resource offer
CreatedAtfield.Note that this implementation likely makes the memory store unsuitable for production use cases. A job creator or resource provider could forge timestamps to game the system.
Related issues or PRs
Epic: https://github.com/Lilypad-Tech/internal/issues/367