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

fix(community): For IBM implementation rename variables, remove defaults, fix tests and minor docs fixes #7129

Merged
merged 8 commits into from
Nov 11, 2024

Conversation

FilipZmijewski
Copy link
Contributor

Following PR provides following fixes:

  • rename variables to camelCase
  • remove default values for model
  • Update and remove some tests
  • update urls in documentation, shrink outputs

- rename variables to camelCase
- remove default values for model
- Update and remove some tests
- update urls in documentation, shrink outputs
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Oct 31, 2024
Copy link

vercel bot commented Oct 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Nov 11, 2024 9:52pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ⬜️ Ignored (Inspect) Nov 11, 2024 9:52pm

@dosubot dosubot bot added the auto:refactor A large refactor of a feature(s) or restructuring of many files label Oct 31, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Oct 31, 2024
 Fix docs, update urls
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Oct 31, 2024
@FilipZmijewski FilipZmijewski changed the title Refactor existing implementation fix(community): For IBM implementation rename variables, remove defaults, fix tests and minor docs fixes Oct 31, 2024
@FilipZmijewski
Copy link
Contributor Author

@jacoblee93 Could you please have a look at it or assign someone to this PR? Thank you

@jacoblee93
Copy link
Collaborator

Happy to merge since this is a newer integration but noting that this is breaking

@jacoblee93 jacoblee93 added the lgtm PRs that are ready to be merged as-is label Nov 5, 2024
@jacoblee93
Copy link
Collaborator

Looks like you need to update your notebooks, they are failing our CI check:

tmp/ibm.mts (40,5): Object literal may only specify known properties, but 'max_new_tokens' does not exist in type 'WatsonxInputLLM'. Did you mean to write 'maxNewTokens'?
file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101
            const err = new Error("Found type errors in new notebook.");
                        ^

Error: Found type errors in new notebook.
    at checkNotebookTypeErrors (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101:[25](https://github.com/langchain-ai/langchainjs/actions/runs/11612088879/job/32550548449?pr=7129#step:8:26))
    at main (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:4:24)
    at file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:7:10
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12) {
  details: "tmp/ibm.mts (40,5): Object literal may only specify known properties, but 'max_new_tokens' does not exist in type 'WatsonxInputLLM'. Did you mean to write 'maxNewTokens'?"
}

Node.js v18.20.4
tmp/ibm.mts (9,3): Object literal may only specify known properties, and 'idOrName' does not exist in type 'WatsonxInputEmbeddings & WatsonxAuth'.
file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101
            const err = new Error("Found type errors in new notebook.");
                        ^

Error: Found type errors in new notebook.
    at checkNotebookTypeErrors (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101:25)
    at main (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:4:24)
    at file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:7:10
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:[33](https://github.com/langchain-ai/langchainjs/actions/runs/11612088879/job/32550548449?pr=7129#step:8:34)7:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12) {
  details: "tmp/ibm.mts (9,3): Object literal may only specify known properties, and 'idOrName' does not exist in type 'WatsonxInputEmbeddings & WatsonxAuth'."
}

@FilipZmijewski
Copy link
Contributor Author

I fixed the failing notebook

@jacoblee93
Copy link
Collaborator

Now failing lint and:

tmp/ibm.mts (39,3): Type '{ maxNewTokens: number; }' is missing the following properties from type 'WatsonxInputLLM': model, serviceUrl, version
file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101
            const err = new Error("Found type errors in new notebook.");
                        ^

Error: Found type errors in new notebook.
    at checkNotebookTypeErrors (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101:[25](https://github.com/langchain-ai/langchainjs/actions/runs/11701931027/job/32611842791?pr=7129#step:8:26))
    at main (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:4:24)
    at file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:7:10
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12) {
  details: "tmp/ibm.mts (39,3): Type '{ maxNewTokens: number; }' is missing the following properties from type 'WatsonxInputLLM': model, serviceUrl, version"
}

Node.js v18.20.4
tmp/ibm.mts (9,3): Object literal may only specify known properties, and 'idOrName' does not exist in type 'WatsonxInputEmbeddings & WatsonxAuth'.
file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101
            const err = new Error("Found type errors in new notebook.");
                        ^

Error: Found type errors in new notebook.
    at checkNotebookTypeErrors (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/check_notebook_type_errors.js:101:25)
    at main (file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:4:24)
    at file:///home/runner/work/langchainjs/langchainjs/libs/langchain-scripts/dist/notebooks/index.js:7:10
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:[33](https://github.com/langchain-ai/langchainjs/actions/runs/11701931027/job/32611842791?pr=7129#step:8:34)7:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12) {
  details: "tmp/ibm.mts (9,3): Object literal may only specify known properties, and 'idOrName' does not exist in type 'WatsonxInputEmbeddings & WatsonxAuth'."
}

@FilipZmijewski
Copy link
Contributor Author

I am just fixing the notebooks but lint doesn't seem to be a problem with our implementation:

/home/runner/work/langchainjs/langchainjs/libs/langchain-xai/src/tests/chat_models_structured_output.int.test.ts
Error:   2:1  error  'zod-to-json-schema' should be listed in the project's dependencies. Run 'npm i -S zod-to-json-schema' to add it  import/no-extraneous-dependencies 

* Fix notebook

* Fix passing params
@jacoblee93
Copy link
Collaborator

CC @bracesproul above

* Fix notebook

* Fix passing params

* Fix embedding notebook
Copy link

vercel bot commented Nov 6, 2024

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

@FilipZmijewski
Copy link
Contributor Author

CC @bracesproul above

any news why the lint would be failing?

@jacoblee93
Copy link
Collaborator

jacoblee93 commented Nov 11, 2024

Have been told it merging main should fix it, have just done so!

@jacoblee93 jacoblee93 merged commit 14fa210 into langchain-ai:main Nov 11, 2024
35 checks passed
@jacoblee93
Copy link
Collaborator

Thanks!

@FilipZmijewski
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:refactor A large refactor of a feature(s) or restructuring of many files lgtm PRs that are ready to be merged as-is size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants