-
Notifications
You must be signed in to change notification settings - Fork 105
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
Make inferences a separate event #910
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Codecov Report
@@ Coverage Diff @@
## develop-scikitlearn #910 +/- ##
======================================================
Coverage ? 81.79%
======================================================
Files ? 187
Lines ? 19278
Branches ? 3309
======================================================
Hits ? 15769
Misses ? 2593
Partials ? 916 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested a lot of changes here which might be kinda hard to follow this way so we might wanna just pair on this tomorrow if you would find that easier. Overall pretty good. The major issue is the passing of the metadata and mapping here needs to be done via setting an attribute in the wrapper and then referencing that in the instrumentation instead of trying to pass it through the call stack. It looks like you originally start on the right path in your commit history but then deviated from it in a follow up commit but I think it's a relatively quick fix to adjust it. I've left a note in our channel but I'm considering dropping the mapping addition based on what we discussed in our planning meeting earlier today.
This PR makes the following changes as requested from MLops team: