Skip to content

remove kwargs#249

Closed
ericbuckley wants to merge 30 commits into
mainfrom
feature/223-remove-kwargs
Closed

remove kwargs#249
ericbuckley wants to merge 30 commits into
mainfrom
feature/223-remove-kwargs

Conversation

@ericbuckley

@ericbuckley ericbuckley commented Mar 13, 2025

Copy link
Copy Markdown
Collaborator

Description

Removing kwargs from Algorithm, in favor of explicit definitions of the necessary fields. Additionally, a large refactor of the Algorithm parameter sent to linkage, this is now an instance schema.Algorithm not model.Algorithm.

Related Issues

closes #223

Additional Notes

  • Evaluation Context a new section in the Algorithm configuration was created for capturing context parameters. This is very similar to how the existing kwargs functioned, but a) adds structure b) is defined once per Algorithm rather than once per pass.
  • Removing rule function since the removal of dibbs-basic, there is only one evaluation rule. It's been removed from the configuration, since it can no longer be configured.
  • Making evaluator function name easier the inclusion of the "func:..." syntax was to allow for some flexibility in testing the phdi code against RecordLinker. That flexibility is no longer needed, and using an enum name rather than a function path string seems easier for users to grasp. (ie, users should have to know or care where the function lives in the codebase).
  • Swap models.Algorithm for schemas.Algorithm Theres a one way relationship between schemas and models, the former knows about the latter, but not the other way around (necessary to prevent circular imports). This has created some problems when passing models.Algorithm around in the linkage code, for instance we always have to convert a blocking key id/string to a BlockingKey object for evaluation, same goes for Feature. The schemas class has everything we need for parsing the data into the appropriate objects for evaluation, it made a lot of sense and reduced some of the logic needed in recordlinker/linking by making this switch.
  • Removing models.AlgorithmPass. Some of the validation logic was being duplicated in the schemas.Algorithm and models.Algorithm, additionally, any time we use the Algorithm, we always want to take a look at the passes. We're already using JSON heavily to store elements of the configuration, so just leaning more into this strategy for storing the passes.

<--------------------- REMOVE THE LINES BELOW BEFORE MERGING --------------------->

Checklist

Please review and complete the following checklist before submitting your pull request:

  • I have ensured that the pull request is of a manageable size, allowing it to be reviewed within a single session.
  • I have reviewed my changes to ensure they are clear, concise, and well-documented.
  • I have updated the documentation, if applicable.
  • I have added or updated test cases to cover my changes, if applicable.
  • I have minimized the number of reviewers to include only those essential for the review.

Checklist for Reviewers

Please review and complete the following checklist during the review process:

  • The code follows best practices and conventions.
  • The changes implement the desired functionality or fix the reported issue.
  • The tests cover the new changes and pass successfully.
  • Any potential edge cases or error scenarios have been considered.

@ericbuckley ericbuckley self-assigned this Mar 13, 2025
@codecov

codecov Bot commented Mar 13, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.24%. Comparing base (e2f9c50) to head (2e42915).
Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
+ Coverage   97.83%   98.24%   +0.40%     
==========================================
  Files          33       32       -1     
  Lines        1805     1769      -36     
==========================================
- Hits         1766     1738      -28     
+ Misses         39       31       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ericbuckley ericbuckley marked this pull request as ready for review March 18, 2025 19:00
@ericbuckley ericbuckley requested a review from bamader as a code owner March 18, 2025 19:00
@ericbuckley ericbuckley requested a review from m-goggins as a code owner March 18, 2025 19:00
@ericbuckley ericbuckley changed the title Feature/223 remove kwargs remove kwargs Mar 19, 2025
@ericbuckley ericbuckley marked this pull request as draft April 2, 2025 17:44
@ericbuckley

Copy link
Copy Markdown
Collaborator Author

closing, this work was completed in #293, #294, #296 and #344

@ericbuckley ericbuckley closed this May 5, 2025
@ericbuckley ericbuckley deleted the feature/223-remove-kwargs branch May 5, 2025 22:26
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.

remove kwargs from AlgorithmPass

1 participant