Repository navigation
Conversation
|
A note on why this PR shows no checks at all, because that is easy to read as "nothing has happened here yet". No workflow has ever run on it. Every job is held in Since CI could not speak for it, I ran the equivalent locally against
Happy to fold this into a larger cleanup of that function if you would rather not spend a review on a one-line change, or to close it if the duplication is deliberate and I have misread it. |
StaticCommandRunner._execute_func deep-copied kwargs, popped "chart" from the original, then deep-copied again. The first copy was overwritten before it was read, so successful commands paid two full deep copies of the caller's payload where one was needed. For commands that take a data payload, technical, econometrics and quantitative all accept list[Data]. With a 1,000-row payload the copy step falls from 5.83 ms to 2.88 ms, and with 5,000 rows from 30.5 ms to 14.6 ms. Successful command behavior is unchanged: the removed assignment was overwritten before its value was read. Co-Authored-By: Aiden <aiden@weco.ai>
e0e7fbe to
0695c67
Compare
|
Thanks for enabling CI. The Python 3.10-3.14 unit matrix was green on the previous head. I pushed the single Black normalization reported in Black 26.5.1, Ruff 0.15.22, and The CodeQL Python job scanned all 1,294 Python files and failed while GitHub was uploading SARIF with |
Merge develop at 78020ee. Preserve the upstream OBBject guard and retain removal of the redundant kwargs deepcopy. Refs openbq-org#7607. Co-Authored-By: Aiden <aiden@weco.ai>
|
Commit cf44e43 merges All 65 command-runner tests pass ( A local diagnostic through |
StaticCommandRunner._execute_funcdeep-copieskwargs, pops"chart"from the original, then deep-copies again:The first copy is never read — it is overwritten two lines later — so every command dispatch pays two full deep copies of the caller's payload where one is needed. This removes the dead store.
What it costs today
The copy is of
kwargs, so the cost tracks the payload. Forobb.equity.price.historical(...)-style commands the kwargs are a handful of scalars and the redundant copy is 2.7 µs — genuinely irrelevant next to the provider HTTP call, and I would not have opened a PR for that alone.It matters for the commands that take a
datapayload —technical,econometricsandquantitativeall acceptlist[Data]— because there the payload is the dataset and the computation is local, so there is no network call to hide behind. Measured on the region itself, median of 15 samples per round, 3 interleaved rounds:Frequency is one per command dispatch, unconditionally — it is not behind the
chartbranch or any provider check. It also runs synchronously insideasync _execute_func, so under the API server it occupies the event loop for that whole duration.A control confirms where the cost is: keeping both copies but moving the
popfirst measures 5.87 ms at 1,000 rows, i.e. unchanged. Thepopis not the cost; the second copy is.Behaviour
Unchanged. The removed line's value was overwritten before any read, so the surviving copy — taken after
"chart"is popped — is exactly what the rest of the function already used.openbb_platform/core/tests/app/test_command_runner.py: 25 passed, 1 failed. That one failure,test_static_command_runner_chart, reproduces identically on unmodifieddevelopat3e071fccin the same environment, so it is pre-existing and not from this change. I did not touch any test.What I deliberately left out
Several alternatives are faster than this patch and I am not proposing them:
json(1.78 ms) orpickle(0.59 ms) round-trips — both change which payload types survive;datakey, or a shallow copy, or copying only when charting was requested (0.001–0.002 ms) — all alias the caller's data, so a downstream mutation could leak back;deepcopycalls in this file, atmerge_args_and_kwargs, are not dead — their values flow intoparameter_map— so removing them would be a real semantic change rather than a cleanup, and they are out of scope here.This PR is only the dead store.
Exploration record
Twelve variants of this region were implemented and measured, including the ones above that do not qualify and a no-copy floor reference. Three structurally different ways of expressing "copy once" — deleting the dead store, popping from the copy, and a single copy with an explicit memo — land at 2.881 / 2.860 / 2.858 ms, within 0.8% of each other, so there is one improving move here rather than a spectrum. Public record of that exploration: https://dashboard.weco.ai/share/DJSi5Z0pwQGnRZN3ZJhlFAdbCs8GHIX5