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

Model linespan endpoint #601

Merged
merged 14 commits into from
Oct 31, 2023
Merged

Model linespan endpoint #601

merged 14 commits into from
Oct 31, 2023

Conversation

Free-Quarks
Copy link
Collaborator

This is for an added endpoint that takes in a zip file containing a python script and returns the line span the dynamics should be under.

It currently uses an Openai LLM for this, but we will replace that with a custom local model once it's ready.

Copy link
Collaborator

@myedibleenso myedibleenso left a comment

Choose a reason for hiding this comment

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

@Free-Quarks , have you tested this locally with the other project deps? We're using pydantic v2 in this project. I've found that to be incompatible with langchain's prompt templates. To ensure skema isn't going to hit the same issue, can you please add a test that instantiates a prompt template?

@myedibleenso
Copy link
Collaborator

Downgrading pydantic to a different major version might cause unexpected behavior. If you run into this issue in the test, maybe you can get around using the langchain prompt template by creating your own prompt string.

@Free-Quarks
Copy link
Collaborator Author

@Free-Quarks , have you tested this locally with the other project deps? We're using pydantic v2 in this project. I've found that to be incompatible with langchain's prompt templates. To ensure skema isn't going to hit the same issue, can you please add a test that instantiates a prompt template?

Thanks for pointing this out! I am using the generic output parser from langchain, not their pydantic one, so hopefully it won't be an issue. I'm using pydantic just to simplify the final serialized json for FastAPI. But I will test it today.

@Free-Quarks
Copy link
Collaborator Author

@myedibleenso So I was already running on pydantic 2.3, but I also updated to 2.4.1 and tested locally and it still works on that as well. So there shouldn't be an issue with pydantic v2!

skema/rest/tests/test_llms.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@myedibleenso myedibleenso left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @Free-Quarks , for adding those unit tests.

@myedibleenso myedibleenso merged commit 6009eb1 into main Oct 31, 2023
8 checks passed
@myedibleenso myedibleenso deleted the lieffers/llm-morae branch October 31, 2023 21:19
github-actions bot added a commit that referenced this pull request Oct 31, 2023
This is for an added endpoint that takes in a zip file containing a
python script and returns the line span the dynamics should be under.

It currently uses an Openai LLM for this, but we will replace that with
a custom local model once it's ready.

---------

Co-authored-by: Justin <[email protected]> 6009eb1
myedibleenso added a commit that referenced this pull request Oct 31, 2023
## Summary of Changes

#601 introduced LLMs using
`langchain` and the Open AI API. The `llms` extra was included in the
extra `all`, but our docker image only installs `core` to avoid bloat:
https://github.com/ml4ai/skema/blob/3af73f74ec127e0fb4c5abf4d3a8f9bd0eea3e71/Dockerfile.skema-py#L53

This caused an import error that triggered a deployment failure loop.
github-actions bot added a commit that referenced this pull request Oct 31, 2023
## Summary of Changes

#601 introduced LLMs using
`langchain` and the Open AI API. The `llms` extra was included in the
extra `all`, but our docker image only installs `core` to avoid bloat:
https://github.com/ml4ai/skema/blob/3af73f74ec127e0fb4c5abf4d3a8f9bd0eea3e71/Dockerfile.skema-py#L53

This caused an import error that triggered a deployment failure loop. be313e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants