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

Adding support for AI model routing with Not Diamond. #23

Merged
merged 9 commits into from
Nov 7, 2024

Conversation

acompa
Copy link
Contributor

@acompa acompa commented Oct 30, 2024

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!

Copy link
Collaborator

@cmpadden cmpadden left a 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.

@acompa
Copy link
Contributor Author

acompa commented Nov 4, 2024

Okay @cmpadden - think I'm ready for a second set of eyes here. The method docstrings and README might have some inconsistencies, fyi!

@cmpadden
Copy link
Collaborator

cmpadden commented Nov 6, 2024

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:

FAILED dagster_contrib_notdiamond_tests/test_resources.py::test_notdiamond_client_with_config - importlib.metadata.PackageNotFoundError: No package metadata was found for dagster-con...

Still looking into it, but if you have any ideas, please share!


Edit: looks like including setuptools in pyproject.toml helps:

[build-system]
requires = ["setuptools>=42"]
build-backend = "setuptools.build_meta"

Still a couple of test failures though--looking:

FAILED dagster_contrib_notdiamond_tests/test_resources.py::test_notdiamond_client - AssertionError: expected call not found.
FAILED dagster_contrib_notdiamond_tests/test_resources.py::test_notdiamond_client_with_config - AssertionError: expected call not found.

@cmpadden
Copy link
Collaborator

cmpadden commented Nov 6, 2024

Oh! It looks like the dagster-openai integration uses a mock library, and not the version that we're using here. Not sure why that's the case. Hmph.

from mock import ANY, MagicMock, patch

@acompa
Copy link
Contributor Author

acompa commented Nov 7, 2024

Oh! It looks like the dagster-openai integration uses a mock library, and not the version that we're using here. Not sure why that's the case. Hmph.

from mock import ANY, MagicMock, patch

Yeah, and actually this library is not independently supported in modern Python versions. So I'm usingunittest.mock here instead.

Issue seems to be caused by my forgetting of a mock call parameter. I've added it back, and all tests pass again locally.

Copy link
Collaborator

@cmpadden cmpadden left a 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. 🤝

@cmpadden cmpadden merged commit 11fa4cf into dagster-io:main Nov 7, 2024
1 check passed
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