Sxnan commented on PR #567: URL: https://github.com/apache/flink-agents/pull/567#issuecomment-4072987848
@alnzng Thanks for the review! Let me address your questions: --- **Regarding case sensitivity for `FLINK_AGENTS_SKIP_JAR_DOWNLOAD` values:** These values are **case-insensitive**. The code at [`backend.py:109`](https://github.com/apache/flink-agents/blob/cd717224aa6c0ac9e355f14958be3cf8597db588/python/_build_backend/backend.py#L109) uses `.lower()` to normalize the value before comparison: ```python if os.environ.get(_SKIP_ENV_VAR, "").lower() in _TRUTHY_VALUES: ``` So `TRUE`, `True`, `true`, `1`, etc., all work correctly. --- **Regarding `jar_manifest.json` version maintenance:** Yes, we have scripts that automatically maintain the version in `jar_manifest.json`: - [`create_release_branch.sh:69-70`](https://github.com/apache/flink-agents/blob/cd717224aa6c0ac9e355f14958be3cf8597db588/tools/releasing/create_release_branch.sh#L69-L70): Updates version when creating a release branch - [`update_branch_version.sh:59-60`](https://github.com/apache/flink-agents/blob/cd717224aa6c0ac9e355f14958be3cf8597db588/tools/releasing/update_branch_version.sh#L59-L60): Updates version when updating the main branch version --- **Regarding downloading all 5 JARs vs just one:** I agree with your assessment. The current design downloads all 5 JARs (1 common ~110MB + 4 thin JARs ~1MB each), but this only happens once during build time. The total size is reasonable (~114MB), and it simplifies the implementation while ensuring users have all JARs available for any Flink version they choose at runtime. --- **Regarding UX impact questions:** 1. **Local Development Flow:** When using `./tools/build.sh` on a local branch (including `-SNAPSHOT` versions): - Maven builds the JARs locally first - `build.sh` copies them to `python/flink_agents/lib/` - When `build_wheel` runs, `_download_jar` detects the JARs already exist and skips the download (see [`backend.py:152-154`](https://github.com/apache/flink-agents/blob/cd717224aa6c0ac9e355f14958be3cf8597db588/python/_build_backend/backend.py#L152-L154)) - For `-SNAPSHOT` versions, Maven Central doesn't have them, but since local JARs exist, no download is attempted 2. **Distribution:** Correct, we will only publish **sdist** (`.tar.gz`) to PyPI, not wheel files. The wheel is built on the user's machine during `pip install` from sdist, and includes the downloaded JARs. 3. **Dependency Management:** There's no runtime dependency resolution for JARs. The JARs are downloaded at **build time** with **fixed SHA-256 checksums** specified in `jar_manifest.json`. This ensures users get the exact same JARs that were tested at release time - no version drift is possible. Python dependencies are managed normally through `pyproject.toml`. --- **Regarding reverting if PyPI increases size limits:** Even if PyPI increases the size limit, this approach has additional benefits beyond just solving the size constraint: - JARs are downloaded on-demand from Maven Central (authoritative source) - SHA-256 verification ensures users get exactly what was published - Smaller sdist package for faster downloads - Clear separation between Python package and Java artifacts We can evaluate whether to simplify back to bundling JARs if size limits become less restrictive, but the current design has merits beyond just the size workaround. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
