-
Notifications
You must be signed in to change notification settings - Fork 457
fix(langchain): fix patching langchain throws an ImportError
#14687
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
base: main
Are you sure you want to change the base?
Conversation
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 242 ± 3 ms. The average import time from base is: 244 ± 3 ms. The import time difference between this PR and base is: -1.6 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
Performance SLOsComparing candidate sabrenner/langchain-patching-fix (ea05833) with baseline main (1d49072) 📈 Performance Regressions (1 suite)📈 telemetryaddmetric - 30/30✅ 1-count-metric-1-timesTime: ✅ 3.131µs (SLO: <20.000µs 📉 -84.3%) vs baseline: -1.7% Memory: ✅ 32.126MB (SLO: <34.000MB -5.5%) vs baseline: +4.7% ✅ 1-count-metrics-100-timesTime: ✅ 211.544µs (SLO: <250.000µs 📉 -15.4%) vs baseline: -2.2% Memory: ✅ 32.106MB (SLO: <34.000MB -5.6%) vs baseline: +5.1% ✅ 1-distribution-metric-1-timesTime: ✅ 2.890µs (SLO: <20.000µs 📉 -85.6%) vs baseline: -2.1% Memory: ✅ 32.106MB (SLO: <34.000MB -5.6%) vs baseline: +4.9% ✅ 1-distribution-metrics-100-timesTime: ✅ 192.741µs (SLO: <220.000µs 📉 -12.4%) vs baseline: +0.3% Memory: ✅ 32.204MB (SLO: <34.000MB -5.3%) vs baseline: +5.0% ✅ 1-gauge-metric-1-timesTime: ✅ 2.300µs (SLO: <20.000µs 📉 -88.5%) vs baseline: 📈 +10.5% Memory: ✅ 32.067MB (SLO: <34.000MB -5.7%) vs baseline: +4.7% ✅ 1-gauge-metrics-100-timesTime: ✅ 124.285µs (SLO: <150.000µs 📉 -17.1%) vs baseline: -1.0% Memory: ✅ 32.067MB (SLO: <34.000MB -5.7%) vs baseline: +4.7% ✅ 1-rate-metric-1-timesTime: ✅ 3.092µs (SLO: <20.000µs 📉 -84.5%) vs baseline: -2.8% Memory: ✅ 32.145MB (SLO: <34.000MB -5.5%) vs baseline: +4.9% ✅ 1-rate-metrics-100-timesTime: ✅ 215.815µs (SLO: <250.000µs 📉 -13.7%) vs baseline: +0.9% Memory: ✅ 32.145MB (SLO: <34.000MB -5.5%) vs baseline: +4.7% ✅ 100-count-metrics-100-timesTime: ✅ 21.154ms (SLO: <23.500ms -10.0%) vs baseline: -1.2% Memory: ✅ 32.145MB (SLO: <34.000MB -5.5%) vs baseline: +5.1% ✅ 100-distribution-metrics-100-timesTime: ✅ 2.017ms (SLO: <2.250ms 📉 -10.4%) vs baseline: +2.6% Memory: ✅ 32.047MB (SLO: <34.000MB -5.7%) vs baseline: +4.9% ✅ 100-gauge-metrics-100-timesTime: ✅ 1.286ms (SLO: <1.550ms 📉 -17.0%) vs baseline: -0.8% Memory: ✅ 32.067MB (SLO: <34.000MB -5.7%) vs baseline: +4.8% ✅ 100-rate-metrics-100-timesTime: ✅ 2.174ms (SLO: <2.550ms 📉 -14.8%) vs baseline: -1.0% Memory: ✅ 32.086MB (SLO: <34.000MB -5.6%) vs baseline: +4.5% ✅ flush-1-metricTime: ✅ 4.343µs (SLO: <20.000µs 📉 -78.3%) vs baseline: +5.2% Memory: ✅ 32.106MB (SLO: <34.000MB -5.6%) vs baseline: +4.9% ✅ flush-100-metricsTime: ✅ 180.651µs (SLO: <250.000µs 📉 -27.7%) vs baseline: -0.6% Memory: ✅ 32.185MB (SLO: <34.000MB -5.3%) vs baseline: +5.1% ✅ flush-1000-metricsTime: ✅ 2.226ms (SLO: <2.500ms 📉 -11.0%) vs baseline: +1.0% Memory: ✅ 32.932MB (SLO: <34.500MB -4.5%) vs baseline: +5.0% 🟡 Near SLO Breach (5 suites)🟡 djangosimple - 30/30✅ appsecTime: ✅ 20.500ms (SLO: <22.300ms -8.1%) vs baseline: -0.2% Memory: ✅ 65.465MB (SLO: <67.000MB -2.3%) vs baseline: +4.9% ✅ exception-replay-enabledTime: ✅ 1.351ms (SLO: <1.450ms -6.8%) vs baseline: +0.5% Memory: ✅ 64.484MB (SLO: <67.000MB -3.8%) vs baseline: +4.8% ✅ iastTime: ✅ 20.487ms (SLO: <22.250ms -7.9%) vs baseline: ~same Memory: ✅ 65.509MB (SLO: <67.000MB -2.2%) vs baseline: +4.9% ✅ profilerTime: ✅ 15.274ms (SLO: <16.550ms -7.7%) vs baseline: -0.3% Memory: ✅ 53.701MB (SLO: <54.500MB 🟡 -1.5%) vs baseline: +4.7% ✅ resource-renamingTime: ✅ 20.543ms (SLO: <21.750ms -5.5%) vs baseline: -0.1% Memory: ✅ 65.529MB (SLO: <67.000MB -2.2%) vs baseline: +5.0% ✅ span-code-originTime: ✅ 26.198ms (SLO: <28.200ms -7.1%) vs baseline: -0.4% Memory: ✅ 68.186MB (SLO: <69.500MB 🟡 -1.9%) vs baseline: +5.7% ✅ tracerTime: ✅ 20.505ms (SLO: <21.750ms -5.7%) vs baseline: ~same Memory: ✅ 65.506MB (SLO: <67.000MB -2.2%) vs baseline: +5.0% ✅ tracer-and-profilerTime: ✅ 21.991ms (SLO: <23.500ms -6.4%) vs baseline: ~same Memory: ✅ 66.779MB (SLO: <67.500MB 🟡 -1.1%) vs baseline: +5.0% ✅ tracer-dont-create-db-spansTime: ✅ 19.387ms (SLO: <21.500ms -9.8%) vs baseline: +0.4% Memory: ✅ 65.490MB (SLO: <66.000MB 🟡 -0.8%) vs baseline: +4.7% ✅ tracer-minimalTime: ✅ 16.580ms (SLO: <17.500ms -5.3%) vs baseline: -0.2% Memory: ✅ 65.147MB (SLO: <66.000MB 🟡 -1.3%) vs baseline: +4.9% ✅ tracer-nativeTime: ✅ 20.454ms (SLO: <21.750ms -6.0%) vs baseline: -0.4% Memory: ✅ 71.437MB (SLO: <72.500MB 🟡 -1.5%) vs baseline: +4.9% ✅ tracer-no-cachesTime: ✅ 18.443ms (SLO: <19.650ms -6.1%) vs baseline: +0.1% Memory: ✅ 65.511MB (SLO: <67.000MB -2.2%) vs baseline: +5.0% ✅ tracer-no-databasesTime: ✅ 18.762ms (SLO: <20.100ms -6.7%) vs baseline: ~same Memory: ✅ 65.155MB (SLO: <67.000MB -2.8%) vs baseline: +4.8% ✅ tracer-no-middlewareTime: ✅ 20.194ms (SLO: <21.500ms -6.1%) vs baseline: ~same Memory: ✅ 65.542MB (SLO: <67.000MB -2.2%) vs baseline: +5.0% ✅ tracer-no-templatesTime: ✅ 20.320ms (SLO: <22.000ms -7.6%) vs baseline: ~same Memory: ✅ 65.529MB (SLO: <67.000MB -2.2%) vs baseline: +5.0% 🟡 errortrackingdjangosimple - 6/6✅ errortracking-enabled-allTime: ✅ 18.359ms (SLO: <19.850ms -7.5%) vs baseline: +1.7% Memory: ✅ 65.292MB (SLO: <66.500MB 🟡 -1.8%) vs baseline: +5.0% ✅ errortracking-enabled-userTime: ✅ 18.329ms (SLO: <19.400ms -5.5%) vs baseline: +1.4% Memory: ✅ 65.313MB (SLO: <66.500MB 🟡 -1.8%) vs baseline: +4.8% ✅ tracer-enabledTime: ✅ 18.181ms (SLO: <19.450ms -6.5%) vs baseline: +0.7% Memory: ✅ 65.372MB (SLO: <66.500MB 🟡 -1.7%) vs baseline: +5.0% 🟡 flasksimple - 18/18✅ appsec-getTime: ✅ 4.584ms (SLO: <4.750ms -3.5%) vs baseline: +0.4% Memory: ✅ 61.892MB (SLO: <65.000MB -4.8%) vs baseline: +4.7% ✅ appsec-postTime: ✅ 6.571ms (SLO: <6.750ms -2.6%) vs baseline: ~same Memory: ✅ 62.010MB (SLO: <65.000MB -4.6%) vs baseline: +5.0% ✅ appsec-telemetryTime: ✅ 4.574ms (SLO: <4.750ms -3.7%) vs baseline: +0.4% Memory: ✅ 61.932MB (SLO: <65.000MB -4.7%) vs baseline: +4.9% ✅ debuggerTime: ✅ 1.854ms (SLO: <2.000ms -7.3%) vs baseline: -0.1% Memory: ✅ 45.397MB (SLO: <47.000MB -3.4%) vs baseline: +4.8% ✅ iast-getTime: ✅ 1.859ms (SLO: <2.000ms -7.0%) vs baseline: -0.1% Memory: ✅ 42.290MB (SLO: <49.000MB 📉 -13.7%) vs baseline: +4.8% ✅ profilerTime: ✅ 1.913ms (SLO: <2.100ms -8.9%) vs baseline: -0.3% Memory: ✅ 46.596MB (SLO: <47.000MB 🟡 -0.9%) vs baseline: +5.2% ✅ resource-renamingTime: ✅ 3.373ms (SLO: <3.650ms -7.6%) vs baseline: -0.2% Memory: ✅ 52.199MB (SLO: <53.500MB -2.4%) vs baseline: +4.9% ✅ tracerTime: ✅ 3.363ms (SLO: <3.650ms -7.9%) vs baseline: -0.4% Memory: ✅ 52.219MB (SLO: <53.500MB -2.4%) vs baseline: +4.8% ✅ tracer-nativeTime: ✅ 3.371ms (SLO: <3.650ms -7.6%) vs baseline: +0.1% Memory: ✅ 58.010MB (SLO: <60.000MB -3.3%) vs baseline: +4.5% 🟡 otelspan - 22/22✅ add-eventTime: ✅ 45.178ms (SLO: <47.150ms -4.2%) vs baseline: +0.2% Memory: ✅ 45.289MB (SLO: <47.000MB -3.6%) vs baseline: +4.8% ✅ add-metricsTime: ✅ 321.592ms (SLO: <344.800ms -6.7%) vs baseline: +0.6% Memory: ✅ 553.284MB (SLO: <562.000MB 🟡 -1.6%) vs baseline: +5.0% ✅ add-tagsTime: ✅ 290.372ms (SLO: <314.000ms -7.5%) vs baseline: ~same Memory: ✅ 555.360MB (SLO: <563.500MB 🟡 -1.4%) vs baseline: +5.0% ✅ get-contextTime: ✅ 83.183ms (SLO: <92.350ms -9.9%) vs baseline: +0.8% Memory: ✅ 40.227MB (SLO: <46.500MB 📉 -13.5%) vs baseline: +4.6% ✅ is-recordingTime: ✅ 42.986ms (SLO: <44.500ms -3.4%) vs baseline: +0.6% Memory: ✅ 44.547MB (SLO: <47.500MB -6.2%) vs baseline: +4.7% ✅ record-exceptionTime: ✅ 61.871ms (SLO: <67.650ms -8.5%) vs baseline: +0.1% Memory: ✅ 40.517MB (SLO: <47.000MB 📉 -13.8%) vs baseline: +4.5% ✅ set-statusTime: ✅ 48.775ms (SLO: <50.400ms -3.2%) vs baseline: +0.3% Memory: ✅ 44.641MB (SLO: <47.000MB -5.0%) vs baseline: +5.0% ✅ startTime: ✅ 42.324ms (SLO: <43.450ms -2.6%) vs baseline: +0.1% Memory: ✅ 44.568MB (SLO: <47.000MB -5.2%) vs baseline: +4.7% ✅ start-finishTime: ✅ 83.218ms (SLO: <88.000ms -5.4%) vs baseline: +0.2% Memory: ✅ 34.583MB (SLO: <46.500MB 📉 -25.6%) vs baseline: +4.7% ✅ start-finish-telemetryTime: ✅ 84.974ms (SLO: <89.000ms -4.5%) vs baseline: +0.7% Memory: ✅ 34.564MB (SLO: <46.500MB 📉 -25.7%) vs baseline: +4.9% ✅ update-nameTime: ✅ 44.563ms (SLO: <45.150ms 🟡 -1.3%) vs baseline: +0.5% Memory: ✅ 44.924MB (SLO: <47.000MB -4.4%) vs baseline: +4.8% 🟡 packagesupdateimporteddependencies - 24/24✅ import_manyTime: ✅ 155.047µs (SLO: <170.000µs -8.8%) vs baseline: +0.1% Memory: ✅ 37.024MB (SLO: <38.500MB -3.8%) vs baseline: +4.6% ✅ import_many_cachedTime: ✅ 121.690µs (SLO: <130.000µs -6.4%) vs baseline: +0.6% Memory: ✅ 37.077MB (SLO: <38.500MB -3.7%) vs baseline: +5.3% ✅ import_many_stdlibTime: ✅ 1.616ms (SLO: <1.750ms -7.6%) vs baseline: ~same Memory: ✅ 37.360MB (SLO: <38.500MB -3.0%) vs baseline: +5.6% ✅ import_many_stdlib_cachedTime: ✅ 0.968ms (SLO: <1.100ms 📉 -12.0%) vs baseline: ~same Memory: ✅ 37.127MB (SLO: <38.500MB -3.6%) vs baseline: +4.5% ✅ import_many_unknownTime: ✅ 830.846µs (SLO: <890.000µs -6.6%) vs baseline: -0.2% Memory: ✅ 37.455MB (SLO: <38.500MB -2.7%) vs baseline: +5.1% ✅ import_many_unknown_cachedTime: ✅ 787.626µs (SLO: <870.000µs -9.5%) vs baseline: -0.3% Memory: ✅ 37.348MB (SLO: <38.500MB -3.0%) vs baseline: +4.4% ✅ import_oneTime: ✅ 19.876µs (SLO: <30.000µs 📉 -33.7%) vs baseline: +0.8% Memory: ✅ 36.946MB (SLO: <39.000MB -5.3%) vs baseline: +3.8% ✅ import_one_cacheTime: ✅ 6.251µs (SLO: <10.000µs 📉 -37.5%) vs baseline: -0.3% Memory: ✅ 37.825MB (SLO: <38.500MB 🟡 -1.8%) vs baseline: +6.7% ✅ import_one_stdlibTime: ✅ 18.878µs (SLO: <20.000µs -5.6%) vs baseline: +0.3% Memory: ✅ 37.673MB (SLO: <38.500MB -2.1%) vs baseline: +6.7% ✅ import_one_stdlib_cacheTime: ✅ 6.265µs (SLO: <10.000µs 📉 -37.4%) vs baseline: ~same Memory: ✅ 37.001MB (SLO: <38.500MB -3.9%) vs baseline: +5.2% ✅ import_one_unknownTime: ✅ 45.387µs (SLO: <50.000µs -9.2%) vs baseline: -0.2% Memory: ✅ 36.982MB (SLO: <38.500MB -3.9%) vs baseline: +4.9% ✅ import_one_unknown_cacheTime: ✅ 6.284µs (SLO: <10.000µs 📉 -37.2%) vs baseline: +0.2% Memory: ✅ 36.957MB (SLO: <38.500MB -4.0%) vs baseline: +4.8%
|
looks like the |
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.
If the test suites are being run duplicate, and considering langchain_community is fairly dependent on langchain_core, should we just combine the two test suites into one langchain test suite?
i tried this, but one of the ci jobs complained about
at which point i was like well i guess it maybe makes sense to split them entirely, or do some weird thing where they have two different venvs in the riotfile but use the same test suite? but i wasn't sure if that would be more complicated then just splitting them 😭 |
…/dd-trace-py into sabrenner/langchain-patching-fix
…langchain-patching-fix
…/dd-trace-py into sabrenner/langchain-patching-fix
llm_with_tools.invoke("What is the sum of 1 and 2?") | ||
|
||
|
||
@pytest.mark.snapshot |
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.
removed this faiss
and pinecone
vectorstore tests in favor of an in-memory vectorstore test (so we don't need to make unnecessary cassettes).
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.
Couple questions but looks great!
def traced_embedding(langchain_core, pin, func, instance, args, kwargs): | ||
provider = instance.__class__.__name__.split("Embeddings")[0].lower() | ||
if provider == "openai" and func.__name__ == "embed_query": | ||
return func(*args, **kwargs) # we previously did not trace OpenAIEmbeddings.embed_query |
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.
I'm fine if we leave this out of this PR, but I'm wondering if we should..?
if langchain_community: | ||
_patch_embeddings_and_vectorstores() | ||
def patched_embeddings_init_subclass(func, instance, args, kwargs): | ||
try: |
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.
do we need this try-finally? Should we only do a try/catch on patching-specific code after func(...)
is called?
# `sys.modules`. Additionally, changing to "session" will fail snapshot tests. | ||
# Setting this to "module" seems to work the best. | ||
# Additionally, this likely isn't an indication of a bug in our code, but how patching happens during tests. | ||
# In userland, patching will really only happen once, and unpatching almost never. |
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.
# In userland, patching will really only happen once, and unpatching almost never. |
This comment is fine without this!
Description
Changes our patching logic to only patch
langchain_core
, as we have not supported operations onlangchain
directly for some time, and is being removed in all langchain 1.0.0 releases - patching this was causingImporError
s due to circular imports from how langchain switched some imports from inlined to top-level.The reason we patched
langchain-community
,langchain-openai
, andlangchain-pinecone
in the first place was because we could not patch the underlyingembed_query
,embed_documents
, andsimilarity_search
for embeddings and vectorstores as the base classes are abstract. However, by patching__init_subclass__
, we can successfully patch them on each instance, meaning we only need to patchlangchain_core
.This is reflected in the change in
_monkey.py
, and the importedlangchain_core
in the patching file.Testing
Updated all riot lockfiles and ensured existing tests continued to pass. In order to make tests work (as
__init_submodules__
did not play nicely with cached modules & patching/unpatching every test), thelangchain_core
fixture is scoped to each module.Additionally, running this script:
with
$ python -m venv .venv $ source .venv/bin/activate $ pip install langchain_openai $ pip install ddtrace
Should not experience any spans being submitted, but with
checked out to this branch should see spans being submitted.
Trace using
ddtrace
without the fix (just OpenAI span, missing LangChain span. Output does not directly include theImportError
butbreakpoint
ing in does reveal it).Trace using local
ddtrace
with the fix (Includes the LangChain parent span).Risks
Importing
langchain
only/directly will no longer trigger patching. With that being said, that hasn't been a valid way to use langchain for some time now.Additional Notes
Fixes #14590