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

Remove unnecessary poetry.lock file and fix tests in OpenAI SDK cookbook code #46

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

smurching
Copy link
Collaborator

@smurching smurching commented Nov 19, 2024

Related Issues/PRs

This PR is a follow-up to #45

What changes are proposed in this pull request?

See title; removes an unnecessary large file & fixes tests in OpenAI SDK cookbook sample code

How is this PR tested?

  • Updated unit tests and verified they pass on this PR
  • Verified there are no references to the poetry.lock file that's removed in this PR

Signed-off-by: Sid Murching <[email protected]>
Signed-off-by: Sid Murching <[email protected]>
Signed-off-by: Sid Murching <[email protected]>
Signed-off-by: Sid Murching <[email protected]>
@smurching smurching requested a review from epec254 November 19, 2024 05:06

CATALOG = "ep"
SCHEMA = "cookbook_local_test"


if os.getenv("GITHUB_ACTIONS") == "true":
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As an alternative, we could mock out the WorkspaceClient APIs & DatabricksFunctionClient in these tests, but skipping these in CI for now for simplicity. It also seems important to actually be able to test UC tool execution against a real backend, given that there are currently differences between local & remote execution. If there weren't, I guess we could just test the actual function definition and assume UC execution works fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@@ -2,7 +2,14 @@ jupyter-book
livereload
black
pytest
mlflow-skinny[databricks]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed? at least in the openai SDK folder, I was using poetry to install these locally and had mirrored that to the requirements.txt files there...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, this is the combined requirements that are installed in CI - but it makes sense to actually mirror user-facing env setup steps in CI (i.e. cd into openai_sdk_agent_app_sample_code and install dependencies via poetry). I'm hoping we can still do that effectively without checking in poetry.lock, unless that buys us better reproducibility than installing python libs via requirements.txt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(will merge for now to fix CI and see if I can make the CI setup match the dev setup in a followup)

@smurching smurching merged commit 4de9cc8 into databricks:main Nov 19, 2024
2 checks passed
fflory pushed a commit to fflory/genai-cookbook that referenced this pull request Dec 12, 2024
…ook code (databricks#46)

* Fix tests

Signed-off-by: Sid Murching <[email protected]>

* Update reqs

Signed-off-by: Sid Murching <[email protected]>

* remove dbconnect dep

Signed-off-by: Sid Murching <[email protected]>

* Skip some tests in GHA

Signed-off-by: Sid Murching <[email protected]>

---------

Signed-off-by: Sid Murching <[email protected]>
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