Skip to content

library settings: allow extra #196

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

Conversation

antoinejeannot
Copy link
Contributor

Hi 👋

This PR's only aim is to allow extra fields as part of LibrarySettings.

This feature comes from the fact that we should be able to store custom settings when instantiating a ModelLibrary with a LibrarySettings.

A few use-cases I have come across:

  • sharing an aiohttp ClientSession to all async models of my ModelLibrary, as recommended by the reference
  • sharing the main thread event loop
  • TL;DR: sharing settings instantiated and set up at startup time

Looking forward to your feedback

Copy link
Contributor

@ldeflandre ldeflandre left a comment

Choose a reason for hiding this comment

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

simple and efficient.

In the future, we could support all this async session and event loop sharing more explicitly so we can fix the unconsistent behaviors of Async models.

What do you think ?

@antoinejeannot
Copy link
Contributor Author

antoinejeannot commented Sep 12, 2023

Async can get very tricky when using modelkit, as experienced by our current initiatives.

It's certain that there is some room for improvements, I will definitely give it a shot in a few :)

Thanks!

@antoinejeannot antoinejeannot merged commit c2713c0 into Cornerstone-OnDemand:main Sep 12, 2023
@antoinejeannot antoinejeannot deleted the allow-extra-library-settings branch September 22, 2023 08:01
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.

2 participants