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

WIP: enabling hooks for KedroPipelineModel serving #608

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Lasica
Copy link

@Lasica Lasica commented Oct 31, 2024

Description

I really wanted a functionality of pandera hook enabled in the model server, so I implemented what was neccesary to make it happen. This PR is not ready yet, but is a point for discussion on whether/how to implement this properly. Addresses #404

Development notes

Added optional hooks param to KedroPipelineModel, model factory and handling of that in mlflow_hook. Pickling hook classes as artifacts wasn't necessary, as they get saved alongside the pipeline pickle.

Only partial hook functionality was enabled - mostly before/after pipeline, node, dataset hooks - after context created and after catalog created were ommitted, as it would require compromises or mocking. This enables you to write your own hook that uses those endpoints to include it alongside the model in the server.

Tested it with my own project and KedroPandera hook - also copying metadata of catalog entries was necessary to make it work.

Checklist

  • Read the contributing guidelines
  • Open this PR as a 'Draft Pull Request' if it is work-in-progress
  • Update the documentation to reflect the code changes
  • Add a description of this change and add your name to the list of supporting contributions in the CHANGELOG.md file. Please respect Keep a Changelog guidelines.
  • Add tests to cover your changes

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@Lasica Lasica marked this pull request as draft October 31, 2024 12:25
@Lasica
Copy link
Author

Lasica commented Oct 31, 2024

Just wanted to hint/share what I did, I'll get to update docs and make it proper if we agree to go on with this.

@Calychas
Copy link
Contributor

Also worth discussing how #580 (the hooks part) would relate to this PR.

@Galileo-Galilei
Copy link
Owner

Hi, I've just updated #445 to give my view on this. Basically, I want to complete #580 first on a different scope (only modify predict and not all other function, but give ability to pass more things). Happy to discuss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

4 participants