-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding support for AI model routing with Not Diamond. #23
Conversation
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.
Looking really good!
Minor change requests:
- Using the templates in GitHub actions
- Updating references to package name as
dagster_contrib_notdiamond
Thanks again for taking the time to contribute, and work with the new integrations repo. Let me know if anything has been confusing.
libraries/dagster-contrib-notdiamond/dagster_contrib_notdiamond/version.py
Outdated
Show resolved
Hide resolved
libraries/dagster-contrib-notdiamond/dagster_contrib_notdiamond/__init__.py
Outdated
Show resolved
Hide resolved
libraries/dagster-contrib-notdiamond/dagster_contrib_notdiamond/__init__.py
Outdated
Show resolved
Hide resolved
libraries/dagster-contrib-notdiamond/dagster_contrib_notdiamond/resources.py
Outdated
Show resolved
Hide resolved
libraries/dagster-contrib-notdiamond/dagster_contrib_notdiamond/resources.py
Outdated
Show resolved
Hide resolved
libraries/dagster-contrib-notdiamond/dagster_contrib_notdiamond/resources.py
Outdated
Show resolved
Hide resolved
libraries/dagster-contrib-notdiamond/dagster_contrib_notdiamond/resources.py
Outdated
Show resolved
Hide resolved
Okay @cmpadden - think I'm ready for a second set of eyes here. The method docstrings and README might have some inconsistencies, fyi! |
This is looking really fantastic. I'm attempting to run the unit tests locally, and am getting a weird setuptools error:
Still looking into it, but if you have any ideas, please share! Edit: looks like including setuptools in pyproject.toml helps:
Still a couple of test failures though--looking:
|
Oh! It looks like the
|
Yeah, and actually this library is not independently supported in modern Python versions. So I'm using Issue seems to be caused by my forgetting of a mock call parameter. I've added it back, and all tests pass again locally. |
f2bb2fc
to
ab27b4c
Compare
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.
Checks are running green--will follow up with you in Slack on next steps. 🤝
Based off of the existing OpenAI integration.
This PR is a WIP for the moment while I update the docs and run integration tests. But feel free to review if you'd like!