Skip to content

Commit 942ac8a

Browse files
Sravan1011claude
andcommitted
Address review: hash installed files not versions; fix loop/import ordering
Build fingerprint (blocking finding): now hashes the on-disk content of every file in the installed moss and inferedge-moss-core distributions (Python sources, data, and native .so/.pyd bindings) instead of just their version strings. A native binding rebuilt without a version bump — e.g. a locally compiled wheel, or a binding change shipped under an unchanged pin — previously kept the old fingerprint and could reuse a stale index; it now changes the file bytes and forces a rebuild. MossClient construction moved inside the moss_client fixture's _setup() coroutine, after _run() has created and installed the shared event loop, so any async initialization the client performs binds to the loop it will actually run on. Made python-dotenv optional in test_bench_ci_moss.py (try/except ModuleNotFoundError) so a repo-wide pytest run that collects this module doesn't hard-fail on an unrelated missing dependency before the credential-based skips even run. Extended ALLOW_BENCHMARK_SKIP to also cover same-repo Dependabot PRs, which GitHub denies normal secrets to just like fork PRs, but which the previous fork-only check would have made fail as if secrets were misconfigured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent ad59ba0 commit 942ac8a

3 files changed

Lines changed: 45 additions & 24 deletions

File tree

.github/workflows/benchmark.yml

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,13 @@ jobs:
5454
env:
5555
MOSS_PROJECT_ID: ${{ secrets.MOSS_PROJECT_ID }}
5656
MOSS_PROJECT_KEY: ${{ secrets.MOSS_PROJECT_KEY }}
57-
# Fork PRs cannot read repository secrets, so missing credentials
58-
# are expected there and the suite may skip. On trusted runs
59-
# (push to main, same-repo PRs, manual dispatch) missing secrets
60-
# make the suite FAIL instead of passing as a green no-op.
61-
ALLOW_BENCHMARK_SKIP: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) && '1' || '0' }}
57+
# Fork PRs, and same-repo Dependabot PRs (which GitHub also denies
58+
# normal secrets), cannot read repository secrets, so missing
59+
# credentials are expected there and the suite may skip. On other
60+
# trusted runs (push to main, same-repo human PRs, manual
61+
# dispatch) missing secrets make the suite FAIL instead of
62+
# passing as a green no-op.
63+
ALLOW_BENCHMARK_SKIP: ${{ (github.event_name == 'pull_request' && (github.event.pull_request.head.repo.full_name != github.repository || github.actor == 'dependabot[bot]')) && '1' || '0' }}
6264
run: |
6365
# Baseline-update runs skip the regression comparison: comparing
6466
# against the baseline being replaced would fail the run (and skip

benchmarks/ci/bench_queries.py

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -42,28 +42,36 @@ def corpus_signature(docs: list[dict[str, Any]]) -> str:
4242

4343

4444
def build_fingerprint() -> str:
45-
"""Fingerprint of the code path that builds the index.
46-
47-
Covers the installed SDK/bindings versions and, when running from the
48-
repository, a content hash of the Python SDK source tree. Any change to
49-
the indexing/build path yields a new fingerprint — and therefore a new
50-
index name via ``index_name_for`` — so the benchmark rebuilds the index
51-
and exercises ``create_index``/document serialization instead of loading
52-
an index built by older code (which could pass on stale embeddings).
45+
"""Fingerprint of every file belonging to the packages that build the index.
46+
47+
Hashes the on-disk content of every file in the installed ``moss`` and
48+
``inferedge-moss-core`` distributions — Python sources, data files, and
49+
native bindings (``.so``/``.pyd``) alike — rather than just their
50+
version strings. A rebuilt native binding (e.g. a locally compiled wheel
51+
during development, or a binding change that ships under an unchanged
52+
version pin) still changes the file bytes even when the version string
53+
doesn't, so it isn't missed. Any change to the indexing/build path
54+
yields a new fingerprint — and therefore a new index name via
55+
``index_name_for`` — so the benchmark rebuilds the index and exercises
56+
``create_index``/document serialization instead of loading an index
57+
built by older code (which could pass on stale embeddings).
5358
"""
54-
from importlib.metadata import PackageNotFoundError, version
59+
from importlib.metadata import PackageNotFoundError, distribution
5560

5661
h = hashlib.sha256()
57-
for pkg in ("moss", "inferedge-moss", "inferedge-moss-core"):
62+
for pkg in ("moss", "inferedge-moss-core"):
5863
try:
59-
h.update(f"{pkg}={version(pkg)}".encode())
64+
dist = distribution(pkg)
6065
except PackageNotFoundError:
61-
pass
62-
sdk_src = Path(__file__).resolve().parents[2] / "sdks" / "python" / "sdk" / "src"
63-
if sdk_src.is_dir():
64-
for p in sorted(sdk_src.rglob("*.py")):
65-
h.update(str(p.relative_to(sdk_src)).encode())
66-
h.update(p.read_bytes())
66+
continue
67+
h.update(f"{pkg}={dist.version}".encode())
68+
for rel_path in sorted(dist.files or (), key=str):
69+
try:
70+
data = rel_path.read_binary()
71+
except (FileNotFoundError, IsADirectoryError, OSError):
72+
continue
73+
h.update(str(rel_path).encode())
74+
h.update(data)
6775
return h.hexdigest()[:12]
6876

6977

benchmarks/ci/test_bench_ci_moss.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,13 @@
3838
load_corpus_slice,
3939
query_set_hash,
4040
)
41-
from dotenv import load_dotenv
41+
try:
42+
from dotenv import load_dotenv
43+
except ModuleNotFoundError: # pragma: no cover - optional outside benchmarks/ci
44+
45+
def load_dotenv() -> None:
46+
return None
47+
4248

4349
load_dotenv()
4450

@@ -204,10 +210,15 @@ def moss_client(corpus_slice, corpus_sig, build_fp):
204210
# native bindings may not be installed in every env.
205211
from moss import DocumentInfo, MossClient
206212

207-
client = MossClient(project_id, project_key)
208213
index_name = os.getenv("MOSS_INDEX_NAME") or index_name_for(corpus_sig, build_fp)
209214

210215
async def _setup():
216+
# Construct the client inside the coroutine, after _run() has
217+
# created and installed the shared event loop — MossClient may bind
218+
# an async session or call get_event_loop() internally, and must do
219+
# so against the loop it will actually run on.
220+
client = MossClient(project_id, project_key)
221+
211222
# Determine existence explicitly (rather than treating any get_index
212223
# failure as "missing") so auth/network errors surface instead of
213224
# silently triggering index creation.

0 commit comments

Comments
 (0)