Eng 376 Benchmark Concept queries #483
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
maparent
force-pushed
the
eng-898-create-database-query-functions-for-base-dg-queries
branch
from
ac99941 to
71e4ccf
Compare
maparent
force-pushed
the
eng-376-run-simulation-on-db-schema
branch
2 times, most recently
from
a6b0d11 to
946b257
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a Python PostgreSQL benchmarking/seeding script, a YAML benchmark configuration, a TypeScript bench runner that queries concepts in a Roam-based space, and an npm script to invoke the bench runner. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant YAML as Config (benchmark.yaml)
participant Py as benchmark.py
participant PG as PostgreSQL
User->>Py: Run main(configPath)
Py->>YAML: Load database_url, schemas, specs
alt Schemas provided
Py->>PG: Drop/Create DB (psql)
else No schemas
Py->>PG: Truncate domain tables (psql)
end
Py->>PG: Insert Space
Py->>PG: Upsert Accounts (batch)
Py->>PG: Upsert Concept Schemata (nodes/relations)
Py->>PG: Upsert Content (batch)
Py->>PG: Upsert Concept Nodes (batch, link content)
Py->>PG: Upsert Relations (batch, link nodes/content)
PG-->>Py: IDs and upsert results
Py-->>User: Print created entity IDs
note over Py,PG: Helpers handle batching, ID propagation, assertions
sequenceDiagram
autonumber
actor Dev as Developer
participant TS as scripts/bench.mts
participant Env as config()
participant API as Platform API (Roam)
participant DB as Supabase Client
Dev->>TS: npm run bench
TS->>Env: Load env, set platform "Roam"
TS->>API: fetchOrCreateSpaceDirect(test creds)
API-->>TS: spaceId or error
alt spaceId ok
TS->>DB: createLoggedInClient(spaceId)
DB-->>TS: authenticated client
TS->>API: getSchemaConcepts(spaceId)
loop For each predefined query
TS->>API: getConcepts(query + {supabase, spaceId})
API-->>TS: Results
TS-->>Dev: Log duration, description, results
end
else failure
TS-->>Dev: Exit with error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
packages/database/benchmark.yaml (1)
16-31: Make benchmark scale tunable (seed, batch size, space identifiers).Add knobs so runs are reproducible and adjustable without code edits.
Consider adding:
- seed: integer
- batch_size: integer (defaults to 500)
- space:
- url: "test"
- name: "test"
- platform: "Roam"
Python can then read these with sensible defaults.packages/database/scripts/bench.mts (1)
24-41: Parameterize space/platform/password via env to avoid hard-coding.Keep defaults but allow overrides for CI and local runs.
Apply this diff:
-config(); -const platform = "Roam"; -let supabase = createClient(); +config(); +const platform = process.env.BENCH_PLATFORM ?? "Roam"; +const spaceUrl = process.env.BENCH_SPACE_URL ?? "test"; +const spaceName = process.env.BENCH_SPACE_NAME ?? "test"; +const password = process.env.BENCH_PASSWORD ?? "password"; +let supabase = createClient(); if (!supabase) process.exit(1); const { data, error } = await fetchOrCreateSpaceDirect({ - url: "test", - name: "test", - platform, - password: "password", + url: spaceUrl, + name: spaceName, + platform, + password, }); if (error || !data || !data.id) { console.error("Could not create space connection", error); process.exit(1); } const spaceId = data.id; -supabase = await createLoggedInClient(platform, spaceId, "password"); +supabase = await createLoggedInClient(platform, spaceId, password); if (!supabase) process.exit(1);packages/database/benchmark.py (4)
132-133: Reduce log volume for large inserts.Printing tens of thousands of IDs floods stdout; print counts instead.
- print("Content:", ", ".join(str(c["id"]) for c in all_content)) + print(f"Content: inserted {len(all_content)} items")
215-216: Reduce log volume for large node inserts.- print("Nodes:", ", ".join(str(n["id"]) for n in all_nodes)) + print(f"Nodes: inserted {len(all_nodes)} items")
266-267: Reduce log volume for large relation inserts.- print("Relations:", ", ".join(str(r["id"]) for r in all_relns)) + print(f"Relations: inserted {len(all_relns)} items")
1-1: Shebang vs executable bit.Either make the file executable (
chmod +x) or drop the shebang to appease linters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/database/benchmark.py(1 hunks)packages/database/benchmark.yaml(1 hunks)packages/database/package.json(1 hunks)packages/database/scripts/bench.mts(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
packages/database/benchmark.yaml
[medium] 13-14: Basic Auth Credentials
(CKV_SECRET_4)
🪛 Ruff (0.14.0)
packages/database/benchmark.py
1-1: Shebang is present but file is not executable
(EXE001)
45-45: subprocess call: check for execution of untrusted input
(S603)
91-91: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
105-105: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
227-227: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
236-236: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
238-238: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
🔇 Additional comments (1)
packages/database/package.json (1)
29-29: LGTM: bench entry point added.Script wiring is correct and tsx is present in devDependencies.
There was a problem hiding this comment.
The main goal of ENG-376 was to "Run Simulation on db schema". In that I would expect to see
- a list of queries that were run
- the results of the benchmark for those queries
Could you put those in either the body of this PR or in the comment of said task, please?
Feel free to use this table format
| Query Name | Benchmark Result (ms) | Notes |
|---|---|---|
| Discourse context: all nodes in any relation with node X | — | |
| All discourse nodes | — | |
| All discourse nodes of type X | — | |
| All discourse relations | — | |
| Relations containing discourse node of type X | — | |
| Relations containing specific discourse node X | — | |
| Nodes that support a claim OR inform a question (authored by user X) | — |
|
That table (in text form) is in ENG-950. The last one was not checked, could be added. (Do you mean any question authored by X or some specific question authored by X?) Without the optimizations, anything having to relations times out, but I'll recalculate. |
|
It was just an example table. That is fine, I'll add it to the main ticket. |
There was a problem hiding this comment.
From the list of primary queryies we were running the benchmark on, it looks like some of these benchmarks were changed slightly. Below is my understand of the mapping, please correct me if I am wrong:
1:1
- query all discourse nodes
- query all discourse relations
- query all discourse nodes of type x
mapped
- discourse context: query all nodes in any relation with node x, and the relation label
- In relation to a specific node
- A specific node's relation
- query all discourse relations containing discourse node of type x
- Query all nodes in some relation to another node of a given type
- query all discourse relations containing specific discourse node x
- same as discourse context
- query all nodes that support a claim OR inform a question and are authored by user x
- not included
Could you please include query all nodes that support a claim OR inform a question and are authored by user x ?
Stated differently: "return all nodes that ... "
Authored by user x
AND
Support any claim
OR
Inform any question
To clarify "Support any claim": claims could part of two separate relations, in this case defined by target relation destination eg:
- evidence supports claim
- claim supports question
So the above query is looking for 1), not 2), where claim is the destination not the target of a supports relation
|
Ok, thanks for the clarification, I think I got it. As stated, this is not something that the query builder can handle. I can filter on relation type AND on other node's type independently; but not on a combination yet. Also, I do not take direction into account yet, but I suspect it's not necessary yet. To be clear, I see this as an important use case, but did not see this as part of the initial list. |
|
If you are saying that this query cannot be adequately supported via the current implementation, then that conclusion is all we are looking for in this test. This will help determine what steps we need to take moving forward. I will update the table in linear to include this determination. |
maparent
force-pushed
the
eng-376-run-simulation-on-db-schema
branch
2 times, most recently
from
91926ec to
3a6d663
Compare
maparent
force-pushed
the
eng-376-run-simulation-on-db-schema
branch
from
3a6d663 to
e53c99a
Compare
2 participants
Summary by CodeRabbit
New Features
Chores
Note: The benchmark is not fullly automated, but detailed instructions are provided at the top of the
scripts/bench.mtsfile.The benchmark as defined here would mostly time out, we need optimizations of ENG-950 to make it work.