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

Refactor: Improve logic for parsing google-Style docstrings #28730

Merged
merged 35 commits into from
Dec 18, 2024

Conversation

isatyamks
Copy link
Contributor

Thank you for contributing to LangChain!

  • PR title: "package: description"
    • Where "package" is whichever of langchain, community, core, etc. is being modified. Use "docs: ..." for purely docs changes, "infra: ..." for CI changes.
    • Example: "community: add foobar LLM"

Description:
Improved the _parse_google_docstring function in langchain/core to support parsing multi-paragraph descriptions before the Args: section while maintaining compliance with Google-style docstring guidelines. This change ensures better handling of docstrings with detailed function descriptions.

Issue:
Fixes #28628

Dependencies:
None.

Twitter handle:
@isatyamks

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 15, 2024
Copy link

vercel bot commented Dec 15, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Dec 18, 2024 5:55am

@dosubot dosubot bot added the Ɑ: core Related to langchain-core label Dec 15, 2024
@efriis efriis self-assigned this Dec 15, 2024
Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

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

could you add some unit tests on cases that currently fail?

@isatyamks
Copy link
Contributor Author

could you add some unit tests on cases that currently fail?

Hi @ccurme,Thank you for the feedback! I’m not very familiar with writing unit tests for this type of case. Could you please guide me.

@efriis
Copy link
Member

efriis commented Dec 16, 2024

you can add some tests to libs/core/tests/unit_tests/utils/test_function_calling.py that call _parse_google_docstring that fail in the current version of langchain but this PR fixes. i'm curious what this actually fixes because there are a bunch of docstrings that are parsed correctly that have multiple paragraphs e.g. https://python.langchain.com/api_reference/openai/chat_models/langchain_openai.chat_models.azure.AzureChatOpenAI.html

and I'm curious what this aims to fix. If you can do it before the holidays let me know! Otherwise we can close this one and reopen with tests whenever you have time to do so!

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 16, 2024
@isatyamks
Copy link
Contributor Author

@efriis I’ve added tests to validate the updated _parse_google_docstring function, but some checks are failing. Could you help identify what’s going wrong?

@efriis
Copy link
Member

efriis commented Dec 16, 2024

fixed the lint errors but the tests you added are failing. You can run them locally with

cd libs/core
poetry install --with lint,typing,test --sync
make test

and for future reference you can run format/lint (what i fixed) with:

cd libs/core
poetry install --with lint,typing,test --sync
make format lint

@isatyamks
Copy link
Contributor Author

isatyamks commented Dec 17, 2024

fixed the lint errors but the tests you added are failing. You can run them locally with

cd libs/core
poetry install --with lint,typing,test --sync
make test

and for future reference you can run format/lint (what i fixed) with:

cd libs/core
poetry install --with lint,typing,test --sync
make format lint

Thanks for the help! I now have a better understanding of how to do it.

@ccurme ccurme assigned ccurme and unassigned efriis Dec 17, 2024
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Dec 17, 2024
@isatyamks
Copy link
Contributor Author

@efriis, I believe all tests have now passed. Huge thanks to you and @ccurme for your help!

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Dec 18, 2024
@ccurme ccurme merged commit 90f7713 into langchain-ai:master Dec 18, 2024
82 checks passed
@isatyamks isatyamks changed the title refactor: improve docstring parsing logic for Google style Refactor: Improve logic for parsing google-Style docstrings Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ɑ: core Related to langchain-core lgtm PR looks good. Use to confirm that a PR is ready for merging. size:M This PR changes 30-99 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

core: make docstring parsing more robust
3 participants