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

docs: Add how-to guide for SQLDatabaseLoader #27696

Closed

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Oct 28, 2024

Dear Eugene,

coming from GH-16246/GH-18281, this patch adds missing documentation that has still been included in crate-workbench#1. We wanted to break it out of that patch, in order to further minimise its size before submitting it.

With kind regards,
Andreas.

NB: Can I also humbly ask you to review/exercise this for validity, @simonprickett or @kneth? I think the demonstrated code should work with recent vanilla versions of LangChain, with all of SQLite, PostgreSQL, and CrateDB.

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 28, 2024
Copy link

vercel bot commented Oct 28, 2024

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

Name Status Preview Comments Updated (UTC)
langchain ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 28, 2024 11:23pm

@dosubot dosubot bot added community Related to langchain-community Ɑ: doc loader Related to document loader module (not documentation) 🤖:docs Changes to documentation and examples, like .md, .rst, .ipynb files. Changes to the docs/ folder labels Oct 28, 2024
Copy link
Contributor Author

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Submitting just a few comments from self-review.

"cell_type": "markdown",
"metadata": {},
"source": [
"# SQL Database\n",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"# SQL Database\n",
"# SQLDatabaseLoader\n",

Comment on lines +15 to +16
"For talking to the database, the document loader uses the [SQLDatabase]\n",
"utility from the LangChain integration toolkit.\n",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"For talking to the database, the document loader uses the [SQLDatabase]\n",
"utility from the LangChain integration toolkit.\n",
"For talking to the database, the document loader uses the [SQLDatabase]\n",
"utility.\n",

"\n",
"[SQLAlchemy]: https://www.sqlalchemy.org/\n",
"[SQLAlchemy dialects]: https://docs.sqlalchemy.org/en/latest/dialects/\n",
"[SQLDatabase]: https://python.langchain.com/docs/integrations/toolkits/sql_database\n",
Copy link
Contributor Author

@amotl amotl Oct 29, 2024

Choose a reason for hiding this comment

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

Linking to the SQLDatabase Toolkit was probably wrong, so let's better link to the Utility instead?

Suggested change
"[SQLDatabase]: https://python.langchain.com/docs/integrations/toolkits/sql_database\n",
"[SQLDatabase]: https://python.langchain.com/api_reference/community/utilities/langchain_community.utilities.sql_database.SQLDatabase.html\n",

Is there a better piece of dedicated documentation about this component elsewhere in the documentation tree, i.e. are you advising to use a different target link here?

@eyurtsev eyurtsev self-assigned this Oct 29, 2024
@eyurtsev eyurtsev changed the title community: Add documentation about SQLDatabaseLoader docs: Add how-to guide for SQLDatabaseLoader Oct 29, 2024
@eyurtsev
Copy link
Collaborator

Hi @amotl, thank you for taking the initiative to improve the how-to guides!

We don't have good guidelines yet for the how-to guides, but if you can browse through a few (https://python.langchain.com/docs/how_to/), you may get a bit of a sense of the format:

  1. Use a notebook format only here (remove the mdx file).
  2. Add a link to the notebook from the how-to guide index in an appropriate place
  3. The title should be phrased as a how-to question (e.g., "How to ...")
  4. If you can link to conceptual prerequisites (e.g., https://python.langchain.com/docs/how_to/passthrough/). Potentially OK to link to any SQL resources that make sense for folks to know about
  5. Don't comment out the package install (maybe simply skip it if you already have the packages installed)
  6. Review https://diataxis.fr/how-to-guides/ if you're not familiar with diataxis

@amotl
Copy link
Contributor Author

amotl commented Oct 29, 2024

Thank you for your guidance, Eugene. I will rework the patch according to your suggestions, and come back afterwards. The same guidelines will probably also apply to GH-27713.

Both pieces of documentation are coming from initially being conceived around LangChain <0.1 times, where blueprints from other how-to documents have not been so elaborate yet.

Copy link

@kneth kneth left a comment

Choose a reason for hiding this comment

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

LGTM

I would appreciate additional reviewers

@amotl
Copy link
Contributor Author

amotl commented Nov 1, 2024

@kneth: The ingredients of this patch need further love, as outlined by @eyurtsev. That's why we toggled it into draft mode again. Relevant patches are welcome.

@eyurtsev eyurtsev assigned efriis and unassigned eyurtsev Dec 11, 2024
@eyurtsev
Copy link
Collaborator

Hi @efriis re-assigned to you. I'm not sure if this PR is being actively worked on right now (probably on the back burner). But I check the integrations page https://python.langchain.com/docs/integrations/document_loaders/ and don't think we have integration docs for this loader.

I suspect we'll want to match this pattern: https://python.langchain.com/docs/integrations/document_loaders/

@efriis
Copy link
Member

efriis commented Dec 12, 2024

ah yep I'll close this one and discuss with their team on #27710

I think they put up all the PRs at once hence the confusing sequencing, and the new integration process will be much better suited to this!

this one can be reopened after the integration is done/published/docs are in

@efriis efriis closed this Dec 12, 2024
@amotl
Copy link
Contributor Author

amotl commented Dec 12, 2024

Hi. I think this one can stay, as it improves existing functionality with corresponding documentation, that has been missed on the first iteration. I was keeping it in draft mode, to keep tracking it. However, it needs improvements like outlined by @eyurtsev.

@efriis efriis reopened this Dec 12, 2024
@efriis efriis marked this pull request as ready for review December 12, 2024 01:49
@efriis
Copy link
Member

efriis commented Dec 12, 2024

ah I misunderstood. @amotl can we

  • remove the changes to the docs/docs/how_to directory
  • instead add a single ipynb page to docs/integrations/document_loaders/sql_database or something like that (matching the format eugene linked)

@amotl
Copy link
Contributor Author

amotl commented Dec 12, 2024

Hi Erick. Sure. As soon as I will resume working on this patch, I will commandeer it into the directions you are suggesting. Thank you very much.

@efriis
Copy link
Member

efriis commented Dec 13, 2024

great! if it's not in the next week, would recommend closing and reopening when you're ready for a review!

@amotl
Copy link
Contributor Author

amotl commented Dec 13, 2024

Hi Erick. Sure. Let's close, and re-open later.

@amotl amotl closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Related to langchain-community Ɑ: doc loader Related to document loader module (not documentation) 🤖:docs Changes to documentation and examples, like .md, .rst, .ipynb files. Changes to the docs/ folder size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants