-
Notifications
You must be signed in to change notification settings - Fork 1
fix various python3.13 bugs #224
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
Conversation
WalkthroughIntroduces version 0.3.5 across the project, adds a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Application
participant Ver as Version Resolver
participant Git as Git Repo
participant Meta as Package Metadata
participant File as .app-version
User->>App: Start
App->>Ver: get_version()
Ver->>Git: Read HEAD tag or commit
alt Git available
Git-->>Ver: tag name or short SHA
Ver-->>App: version (git)
else Git unavailable/none
Ver->>Meta: importlib.metadata.version("gliner-api")
alt Metadata available
Meta-->>Ver: package version
Ver-->>App: version (package)
else Metadata unavailable
Ver->>File: Read project/.app-version
alt File present/non-empty
File-->>Ver: file version
Ver-->>App: version (file)
else No sources
Ver-->>App: "unknown"
end
end
end
sequenceDiagram
autonumber
actor Op as Operator
participant Main as main.py
participant MS as Metrics Server (:: dual-stack)
Op->>Main: Launch with metrics enabled
Main->>MS: Start listener on ::
Note right of MS: Single thread handles dual-stack
Op-->>Main: Shutdown signal
Main->>MS: shutdown()/close()
Main-->>Op: Exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
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.
Actionable comments posted: 1
🧹 Nitpick comments (13)
.app-version (1)
1-1: Static version fallback added — OK.Works with the new resolution order. Consider adding a trailing newline to satisfy POSIX text-file conventions and some linters.
README.md (3)
209-209: Clarify host default with uvicorn.Empty string binds to all interfaces on most platforms; call it out explicitly to avoid confusion (e.g., “'' (all interfaces, IPv4/IPv6 where supported)”). This matches main.py behavior when starting metrics.
39-41: Fix minor grammar and branding.“in it's” → “in its”; “Huggingface” → “Hugging Face”.
-You can try the live demo of the GLiNER API container in it's Huggingface Space: +You can try the live demo of the GLiNER API container in its Hugging Face Space:
9-9: Keep GLiNER badge in sync with dependency.Badge shows 0.2.21, pyproject pins gliner[stanza]==0.2.22. Update badge to avoid confusion.
-[](https://github.com/urchade/GLiNER) +[](https://github.com/urchade/GLiNER)main.py (1)
28-33: Close the HTTP server socket explicitly on shutdown.Add
server_close()aftershutdown()to promptly release the port; recommended in upstream discussions.@app.on_event("shutdown") async def close_metrics_server(): metrics_server.shutdown() + metrics_server.server_close() metrics_thread.join() logger.info("Prometheus metrics server shutdown complete.")Rationale: prevents lingering binds during rapid restarts. (github.com)
gliner_api/version.py (2)
28-41: Tighten git tag detection and normalize optional “v” prefix.Current loop works but is O(n tags) and returns raw tag (e.g., “v0.3.5”). Optionally strip a leading “v” to align with pyproject’s “0.3.5”.
- current_commit_hash: str = repo.commit("HEAD").hexsha - - # Check if the current commit is tagged - for tag in repo.tags: - if tag.commit.hexsha == current_commit_hash: - return tag.name - - # If the current commit is not tagged, use the commit hash stub - if version is None: - return current_commit_hash[:8] + head = repo.commit("HEAD") + for tag in repo.tags: + if tag.commit == head: + name = tag.name + return name[1:] if name.startswith("v") else name + return head.hexsha[:8]
66-92: File-based fallback — OK; minor path resolution tweak optional.Using
pyproject.tomldiscovery is sensible. ConsiderPath(__file__).resolve()to avoid surprises with symlinks/site-packages.cpu.Dockerfile (6)
47-50: Harden non-root user creation.Create a system user without login shell.
-RUN groupadd -g 1001 appuser && \ - useradd -m -u 1001 -g appuser appuser +RUN groupadd -r -g 1001 appuser && \ + useradd -r -m -u 1001 -g appuser -s /usr/sbin/nologin appuser
52-54: Persist model cache between runs.Add a volume so HF cache survives container restarts.
ENV HF_HOME=/app/huggingface +VOLUME ["/app/huggingface"]
66-66: Avoid globally silencing Python warnings in 3.13 migration.Consider toggling via env for prod only.
-ENV PYTHONWARNINGS="ignore" +ARG PYTHON_WARNINGS=default +ENV PYTHONWARNINGS="${PYTHON_WARNINGS}"
32-42: Optional: enrich OCI labels.Add base name and ref name to aid provenance.
LABEL org.opencontainers.image.version="$IMAGE_VERSION" \ + org.opencontainers.image.ref.name="gliner-api:${IMAGE_VERSION}" \ + org.opencontainers.image.base.name="ghcr.io/astral-sh/uv:python3.13-bookworm-slim" \ org.opencontainers.image.url='https://ghcr.io/freinold/gliner-api' \
71-74: Add a container healthcheck.Helps orchestration detect readiness/failures.
ENTRYPOINT ["python", "main.py"] CMD ["--host", "0.0.0.0", "--port", "8080"] +HEALTHCHECK --interval=30s --timeout=5s --start-period=20s --retries=3 \ + CMD curl -fsS http://127.0.0.1:8080/health || exit 1
1-74: Update .dockerignore: add missing exclusions & remove duplicate entry.dockerignore present; it already excludes __pycache/, .git/, .gitignore, .pytest_cache/, .venv/ (duplicate .venv at lines 10 & 21). Add tests/ and .github/ to shrink the build context; optionally add dist/, build/, node_modules/.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.app-version(1 hunks)README.md(3 hunks)cpu.Dockerfile(2 hunks)gliner_api/version.py(3 hunks)main.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-07T10:44:27.531Z
Learnt from: freinold
PR: freinold/GLiNER-API#86
File: gliner_api/frontend.py:13-13
Timestamp: 2025-07-07T10:44:27.531Z
Learning: In the GLiNER-API project, the frontend and backend are always run on the same machine, with the frontend bound to the same FastAPI backend in main.py. This architectural constraint makes hardcoded localhost connections appropriate and eliminates the need for host configurability in the frontend client.
Applied to files:
README.md
🧬 Code graph analysis (1)
main.py (1)
gliner_api/backend.py (1)
info(219-229)
🔇 Additional comments (15)
pyproject.toml (3)
3-3: Version bump to 0.3.5 — OK.In sync with .app-version and version.py lookup order.
47-55: Verified PyTorch indices.
Both the cu128 index and the cpu index contain torch==2.8.0 wheels built for Python 3.13 (cp313), including CUDA 12.8 (cu128); no changes required.
6-6: Avoid pinning requires-python to an exact patch.
==3.13.7blocks installs on 3.13.8+ and future patches. Prefer a compatible range:-requires-python = "==3.13.7" +requires-python = ">=3.13.7,<3.14"Would you like me to propagate this phrasing to README “Prerequisites” for consistency?
⛔ Skipped due to learnings
Learnt from: freinold PR: freinold/GLiNER-API#221 File: pyproject.toml:6-6 Timestamp: 2025-09-09T18:14:29.664Z Learning: The freinold/GLiNER-API repository uses Renovate for automated dependency management and prefers exact version pinning (e.g., "==3.13.7") rather than version ranges in pyproject.toml.README.md (2)
8-8: Python badge updated — OK.Matches the current toolchain.
241-241: Loosen prerequisite phrasing to match packaging.If you adopt the suggested
>=3.13.7,<3.14range, adjust this line accordingly (e.g., “Python 3.13 (tested on 3.13.7)”).main.py (1)
20-24: Single dual-stack metrics server — OK, matches library behavior.
start_http_server(addr="::", ...)returning(server, thread)is supported and allows graceful shutdown. Note dual-stack availability depends on OS socket options (e.g., IPV6_V6ONLY). Consider a small log hint if IPv4 access isn’t available.Reference: prometheus client docs show returning server and thread and shutting down via
shutdown()andjoin(). (prometheus.github.io)gliner_api/version.py (3)
10-17: Version resolution logging — OK.Clear source attribution; keep call-site frequency low to avoid noisy INFO logs.
Where is
get_version()invoked? If per-request, consider downgrading to DEBUG.
43-46: Good resilience on git lookup.Local import and debug logging are appropriate here.
48-63: Package metadata fallback — OK.Covers installed-package scenarios; broad exception catch is fine.
cpu.Dockerfile (6)
58-60: LGTM: virtualenv PATH wiring is correct.Copies the .venv from builder and prioritizes it in PATH.
61-64: LGTM: log noise controls for tqdm/HF.Reasonable for container logs.
19-24: LGTM: uv two-phase sync for cache efficiency.Pre-fetch with --no-install-project then full sync post-COPY is solid for layer reuse.
17-17: Extras (cpu, frontend) present in pyproject.toml and uv.lock — no change needed.pyproject.toml defines cpu and frontend under [project.optional-dependencies]; uv.lock contains matching [package.optional-dependencies] entries and provides-extras includes "cpu" and "frontend".
72-74: Confirm empty --host handling or use explicit 0.0.0.0README documents
""as "bind to all interfaces" (README.md:208). cpu.Dockerfile currently uses CMD ["--host", "", "--port", "8080"] (cpu.Dockerfile:72-74) and gpu.Dockerfile mirrors it (gpu.Dockerfile:95-97). main.py is not present in the repo so I cannot verify how the CLI parses an empty host — either confirm main.py/uv treats""as bind-all, or replace the CMD with ["--host","0.0.0.0","--port","8080"].
14-18: CI enables BuildKit — no CI changes required.
.github/workflows/docker-release.yml sets up docker/setup-buildx-action and uses docker/build-push-action; cpu.Dockerfile and gpu.Dockerfile use --mount, so the release workflow supports BuildKit mounts.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores