Skip to content

refactor: apply codebase review fixes, add type stubs, and offline unit tests - #65

Merged
gawbul merged 14 commits into
mainfrom
refactor/codebase-review-fixes
Aug 22, 2026
Merged

refactor: apply codebase review fixes, add type stubs, and offline unit tests#65
gawbul merged 14 commits into
mainfrom
refactor/codebase-review-fixes

Conversation

@gawbul

@gawbul gawbul commented Aug 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

This PR addresses all findings from the Staff+ codebase review across architecture, packaging, type safety, error resilience, rate limiting, and test quality, as well as a modernized CI/CD workflow architecture.

Key Changes

  1. CI/CD Pipeline Overhaul:

    • Decoupled PR Workflow: Separated linting, matrix unit tests, and live smoke tests in .github/workflows/pull_request.yaml.
    • Fast Matrix Unit Tests: Offline mocked unit tests run across all 5 Python versions (3.10–3.14) in parallel (~20s total, deterministic, 0% flakiness).
    • Eliminated Concurrent Live Test DDoS: Live API tests now run on a single runner rather than 5 parallel runners, strictly respecting Ensembl's 15 req/s rate limit and preventing 429 errors.
    • Nightly API Drift Check: Added .github/workflows/nightly.yaml to monitor upstream Ensembl REST API changes/deprecations automatically on a nightly cron without blocking PRs.
    • SHA Pinned Actions: Checked and upgraded all GitHub Actions to their latest versions and pinned to exact commit SHAs with version comments.

  2. Rate Limiting & Timing Fixes:

    • Replaced flawed delta calculation with a rolling 1.0s sliding window using collections.deque[float] (reqs_per_sec=15, wall_time=1.0s), avoiding false unneeded sleeps when requests are spread out over time.
    • Fixed isinstance(retry_after, (int, float)) type check in EnsemblRestRateLimitError so integer Retry-After headers are formatted properly.

  3. Parameter Validation & URL Construction:

    • Fixed regex digit exclusion PARAM_REGEX = [a-zA-Z0-9_]+ so parameters containing 0 are matched.
    • Safely URL-quote dynamic path parameters with urllib.parse.quote(..., safe=":").
    • Fixed base URL trailing slash and double-slash bugs.

  4. Packaging & Dependency Hygiene:

    • Moved development tools (mypy, ruff, pre-commit, types-requests) from runtime [tool.poetry.dependencies] to [tool.poetry.group.dev.dependencies].
    • Added responses to [tool.poetry.group.test.dependencies] for mocked unit tests.
    • Updated poetry.lock.

  5. Type Safety & IDE Autocomplete (PEP 561):

    • Added py.typed marker file.
    • Added ensemblrest.pyi type stub explicitly declaring typed signatures, required/optional parameters, and docstrings for all 106 endpoints.
    • Added __dir__() and __getattr__() dynamic reflection on EnsemblRest.

  6. Error Resilience & Resource Management:

    • Wrapped json.loads in safe decode handlers to prevent unhandled JSONDecodeError on HTML error pages or empty bodies.
    • Added close(), __enter__(), and __exit__() context manager support to EnsemblRest.
    • Added specialized exception subclasses (EnsemblRestBadRequestError, EnsemblRestNotFoundError, EnsemblRestTimeoutError).
    • Safely handled package version resolution with PackageNotFoundError fallback in __init__.py.

  7. Comprehensive Offline Unit Test Suite:

    • Added tests/test_unit.py with 28 fast, offline, 100% mocked unit tests using responses.
    • make unit-test now runs completely offline in ~2.3 seconds with 91% code coverage.

  8. Documentation:

    • Moved CLAUDE.md to AGENTS.md and added a referencing pointer in CLAUDE.md.

Pinned Actions Matrix

Action Tag Commit SHA
actions/checkout v7.0.1 3d3c42e5aac5ba805825da76410c181273ba90b1
actions/setup-python v7.0.0 5fda3b95a4ea91299a34e894583c3862153e4b97
snok/install-poetry v1.4.2 a783c322200f0519c7926aa6faa857c4e23e9263
actions/cache v6.1.0 55cc8345863c7cc4c66a329aec7e433d2d1c52a9
coverallsapp/github-action v2.3.8 8d6379e14d29928660c4ba802d8e85393440b329
actions/upload-artifact v7.0.1 043fb46d1a93c77aae656e7c1c64a875d1fc6a0a
actions/download-artifact v8.0.1 3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c
softprops/action-gh-release v3.0.2 3d0d9888cb7fd7b750713d6e236d1fcb99157228

Verification

make format       # Ruff format: clean
make lint         # Ruff lint: all checks passed
make type-check   # Mypy strict mode: 0 issues
make unit-test   # Pytest (offline): 28 passed in 2.26s (91% coverage)

@coveralls

coveralls commented Aug 15, 2026 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage is 91.212% — refactor/codebase-review-fixes into main. No base build found for main.

gawbul and others added 12 commits August 15, 2026 14:53
@gawbul
test: update test_wait4request to verify sliding window rate limiter
a0ef2ba
@gawbul
ci: add workflow concurrency controls, explicit timeouts, and least p…
144aa4e 
…rivilege permissions
@gawbul
ci: allow pytest-rerunfailures to retry any transient live test failure
c3c35be
@gawbul
test: optimize curl options and retry delays in live test helpers
021d350
@gawbul
ci: increase live test job timeout to 90 minutes
190ddab
@gawbul
test: optimize live test timeouts and ensure explicit failures on error
ec905ce
@gawbul
ci: configure 5 reruns with 5s delay, 60s live timeout, and standard …
d38fe51 
…CAFE test query
@gawbul @claude
test: resolve GA4GH feature transcript version dynamically
81774da 
Ensembl bumped ENST00000408937 (FOXP2-210) from version 7 to 8, and
/ga4gh/features/{id} rejects stale versions with a 400. This broke
test_getGA4GHFeatures deterministically, so the live-test reruns could
not recover it.

Add a versionedStableId() helper that looks up the current version via
/lookup/id/ and have test_getGA4GHFeatures build its ID at runtime, so
both the reference curl and the library call stay in sync across future
releases. The helper raises rather than returning an unusable value, to
avoid degrading into a vacuous comparison when the lookup fails.

Also bump the hardcoded .7 to .8 in the examples.py and README.md
snippets so they no longer error when copy-pasted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gawbul @claude
test: give the slow CAFE endpoints a longer timeout
e01f7bc 
The /cafe/ endpoints return the whole species tree (~400 taxa, ~100KB)
and their latency is driven by server load, not payload size: the same
request measured between 9s and 122s across samples, with the smallest
response being the slowest. The 60s budget lost that race often enough
to make the CAFE tests flaky, and the configured reruns could not help
because every attempt hit the same wall.

Raise the global TIMEOUT to 90s and add a CAFE_TIMEOUT of 180s, applied
to all three CAFE tests on both sides of the comparison via a new
optional timeout parameter on launch()/jsonFromCurl(). Only --max-time
is extended; --connect-timeout stays on the global value since the
handshake is not what is slow here.

Also derive the library timeout in setUp() from TIMEOUT rather than
repeating the literal, so the two cannot drift apart.

Note this does not address the intermittent 503s from the same
endpoints, which the existing reruns already handle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gawbul @claude
test: extend the slow-endpoint timeout to the genetree symbol lookup
6e0c34a 
test_getGeneTreeMemberBySymbol hits the same wall the CAFE tests did,
measuring 118s and 140s against the 90s global budget. Measuring the
neighbouring queries shows the previous commit blamed the wrong thing:
the cost is symbol resolution, not payload size or pruning. The same
BRCA2 tree returns byte-identical content in 5.6s via /genetree/id but
takes 176s via /genetree/member/symbol, and a pruned
/genetree/member/id query was the fastest of all at 2.3s.

Rename CAFE_TIMEOUT to SLOW_ENDPOINT_TIMEOUT since it is not specific
to CAFE, raise it to 300s to clear the 176s worst case with margin, and
apply it to test_getGeneTreeMemberBySymbol as well. Correct the comment
to record what was actually measured.

test_getGeneTreeById and test_getGeneTreeMemberById stay on the global
timeout, having measured 5.6s and 13.6s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gawbul @claude
ci: raise live job timeout to 120m and extend the homology symbol test
0f0333f 
The live job timed out at 90 minutes having reached 80%, so raise both
live jobs to 120 minutes. The nightly drift check runs the same suite
against the same API and would have hit the identical wall.

test_getHomologyBySymbol is the last test over the global budget,
measuring 104s and 112s on a cold cache, so give it the extended
timeout as well.

Correct the SLOW_ENDPOINT_TIMEOUT comment. Re-measuring showed these
responses are cached by Ensembl, which made some earlier readings look
fast on a second sample; with untouched genes as controls the symbol
cost holds up, a 929KB homology query by id taking 10.5s against 104s
for a smaller 372KB one by symbol. The rest of the suite's symbol
endpoints are cheap and stay on the global timeout: /xrefs/symbol and
/lookup/symbol both measured around 0.1s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gawbul
test: extend slow-endpoint timeout to GA4GH feature endpoints and inc…
b7f6e47 
…rease to 600s
@gawbul
gawbul merged commit 80aa244 into main Aug 22, 2026
8 checks passed
@gawbul
gawbul deleted the refactor/codebase-review-fixes branch August 22, 2026 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gawbul @coveralls