-
Notifications
You must be signed in to change notification settings - Fork 7
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
[LangChain] Improve Jupyter Notebooks and add software tests #97
Conversation
ce2695f
to
9d6e221
Compare
b9c4123
to
1ff62c7
Compare
Problem-DELETE OK, 30 rows affected (0.000 sec)
+DELETE OK, 0 rows affected (0.000 sec) -- https://github.com/crate/cratedb-examples/actions/runs/6684017889/job/18160822060?pr=97#step:5:585 Sourcecratedb-examples/framework/langchain/document_loader.ipynb Lines 55 to 77 in 1ff62c7
|
To make the output more idempotent, the SQL DDL statements have been adjusted. |
01813ea
to
4116268
Compare
Status QuoIt works. 🚀
-- https://github.com/crate/cratedb-examples/actions/runs/6684315548/job/18161437538?pr=97#step:5:519 Outlook
|
54a08b6
to
fb462c9
Compare
def monkeypatch_pytest_notebook_treat_cell_exit_as_notebook_skip(): | ||
""" | ||
Patch `pytest-notebook`, in fact `nbclient.client.NotebookClient`, | ||
to propagate cell-level `pytest.exit()` invocations as signals | ||
to mark the whole notebook as skipped. | ||
|
||
In order not to be too intrusive, the feature only skips notebooks | ||
when being explicitly instructed, by adding `[skip-notebook]` at the | ||
end of the `reason` string. Example: | ||
|
||
import pytest | ||
if "ACME_API_KEY" not in os.environ: | ||
pytest.exit("ACME_API_KEY not given [skip-notebook]") | ||
|
||
https://github.com/chrisjsewell/pytest-notebook/issues/43 | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a utility temporarily added here. Based on the outcome from chrisjsewell/pytest-notebook#43, it may be added to the upstream library, or to a different spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code has been added to another library, to not clutter the canonical cratedb-examples
repository with such utility code.
-- https://github.com/pyveci/pueblo/blob/v0.0.1/pueblo/testing/notebook.py
"\tid = sa.Column(sa.BigInteger, primary_key=True, default=generate_autoincrement_identifier)\n", | ||
"\tid = sa.Column(sa.BigInteger, primary_key=True, server_default=sa.func.now())\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is so good to have this tested now. 🚀
Even if the Notebook worked once upon a time, it did not in its current form, because the previous implementation changed.
It is difficult to catch such errors without running rigid software testing, minor things like this easily slip through, effectively rendering the notebook unusable for all people trying to exercise it.
"\t# Make sure to start with a blank canvas.\n", | ||
"\tchat_message_history.clear()\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idempotency flaws when testing. Fixed now.
cd92557
to
cd9b202
Compare
fb462c9
to
b246364
Compare
About
This patch adds software tests to the examples around LangChain, in order to validate that the Jupyter Notebooks work well, and to keep them working / get alerted if something breaks.
Backlog
test_notebook[vector_search.ipynb]
is yet untested. It will be addressed on behalf of a subsequent iteration. See [LangChain] Fix testing Jupyter Notebookvector_search.ipynb
#98.Details
This patch builds upon GH-96 and GH-85.
/cc @andnig, @marijaselakovic, @karynzv, @hlcianfagna, @hammerhead, @ckurze, @WalBeh