Olawoyin007 commented on code in PR #888:
URL: https://github.com/apache/flink-agents/pull/888#discussion_r3570517906
##########
tools/install.sh:
##########
@@ -1563,6 +1564,16 @@ check_java() {
return 0
}
+# Upper bound (exclusive) on the Python minor version for the selected
+# Flink Agents release: 0.1.x / 0.2.x publish requires-python >=3.10,<3.12,
+# while 0.3.0+ publishes >=3.10,<3.13 (adds Python 3.12 support).
+python_minor_ceiling() {
+ case "$FLINK_AGENTS_VERSION" in
+ 0.1.*|0.2.*) printf '12' ;;
+ *) printf '13' ;;
+ esac
Review Comment:
Good catch - fixed in 1839dcd. `python_minor_ceiling` now derives the
ceiling from **both** versions and returns the stricter of the two: the Flink
Agents axis (0.1.x/0.2.x → <3.12, 0.3.0+ → <3.13) and a new Flink axis (Flink
<2.1 → <3.12, since Python 3.12 needs Flink 2.1+). The Flink comparison is
major-then-minor numeric (so 1.20.x isn't mistaken for ≥2.1) and reuses
`flink_major_minor`, so snapshots/rc/source builds like `2.1-SNAPSHOT` resolve
correctly; an unparseable `FLINK_VERSION` leaves the Flink axis unrestricted
(fail open). Because `validate_python_bin` and `resolve_python` both consult
the ceiling, they pick this up automatically.
For revalidation on change: `revalidate_python_for_agents_version` is
renamed `revalidate_python_constraint` and now also runs on the `flink_version`
and `install_flink` edit arms (both mutate `FLINK_VERSION`, and
`install_flink`'s No branch re-detects from `FLINK_HOME`). Your exact case -
`0.3.0 + Flink 2.0.x + Python 3.12` - is covered by new bats cases in
`validate_python_bin.bats` and `revalidate_python_constraint.bats`.
One scoping note: the standalone `flink_home` edit arm currently only
repoints `FLINK_HOME` and deliberately does not re-detect the Flink version
(unlike the `install_flink` arm), so I left its semantics unchanged rather than
widen it here. Happy to add version re-detection to that arm too if you'd
prefer it fully symmetric.
##########
tools/install.sh:
##########
@@ -1615,13 +1626,31 @@ resolve_python() {
die "No Python interpreter provided."
fi
if ! validate_python_bin "$input"; then
- die "Provided Python is invalid or unsupported (need >=3.10 and
<3.12): $input"
+ die "Provided Python is invalid or unsupported (need >=3.10 and
<3.$(python_minor_ceiling)): $input"
fi
PYTHON_BIN="$input"
ui_success "Using Python: $PYTHON_BIN"
return 0
}
+# The Python ceiling depends on the selected Flink Agents release (see
+# python_minor_ceiling), so editing the version at the confirm screen can
+# invalidate an interpreter that resolve_python already accepted — e.g.
+# Python 3.12 passes for 0.3.0 but pip install of 0.2.x requires <3.12.
+# Called after a version edit; re-resolves PYTHON_BIN when it no longer
+# satisfies the new release, mirroring the enable_pyflink edit arm.
+revalidate_python_for_agents_version() {
+ if [[ "$ENABLE_PYFLINK" != "Yes" ]] || [[ -z "${PYTHON_BIN:-}" ]]; then
+ return 0
+ fi
+ if validate_python_bin "$PYTHON_BIN"; then
+ return 0
+ fi
+ ui_warn "The selected Python (${PYTHON_BIN}) is incompatible with Flink
Agents ${FLINK_AGENTS_VERSION} (requires Python >=3.10 and
<3.$(python_minor_ceiling))"
+ PYTHON_BIN=""
Review Comment:
Fixed in 1839dcd. You're right that re-resolving `PYTHON_BIN` doesn't help
if an existing venv is reused with its own interpreter. Two layers now:
- `revalidate_existing_venv` (called from `revalidate_python_constraint`, so
it runs on every version edit) checks a reused venv's `bin/python` against the
current ceiling at plan time. If it's incompatible it offers to recreate it or
point at a different path; recreation is **deferred** to `setup_python_env` via
a `RECREATE_VENV` flag (so a later cancel never destroys the venv) and guarded
to a directory carrying a `pyvenv.cfg` marker, so it only ever removes a real
venv. Non-interactive callers get an actionable error instead of a mid-install
failure.
- `setup_python_env` re-validates a reused venv's interpreter as a final
guard for any path that skips plan-time revalidation (e.g. an explicit
`VENV_DIR` in non-interactive mode), failing early with a clear message rather
than at `pip install`.
Your regression scenario (Agents 0.3.0 → 0.2.1, existing Python 3.12 venv)
is covered by new cases in `revalidate_python_constraint.bats`, including the
non-interactive die path. Full suite is 162 passing.
--
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]
