feat(framework): Add startup update checks for flwr#6788
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a startup-only update check to the flwr CLI, intended to print a cached “update available” message and refresh update-check state in the background.
Changes:
- Invoke
warn_if_flwr_update_available(process_name="flwr")at CLI startup. - Implement update-check payload/caching/network refresh logic (and tests) under
flwr.supercore.utils. - Disable the update check in the managed local SuperLink child process to avoid duplicate checks.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/py/flwr/supercore/utils.py | Adds update-check payload creation, cache read/write, refresh request, and startup warning logic. |
| framework/py/flwr/supercore/utils_test.py | Adds unit tests for update-check payload, caching behavior, and refresh behavior via monkeypatching. |
| framework/py/flwr/supercore/constant.py | Adds constants for update-check URL/timeouts/cache path and display interval. |
| framework/py/flwr/cli/app.py | Calls the update-check warning function at CLI startup. |
| framework/py/flwr/cli/cli_test.py | Patches out the update check for most CLI tests; adds a test asserting the callback triggers the check. |
| framework/py/flwr/cli/local_superlink.py | Passes an env var to disable update checks in the managed child process. |
| framework/py/flwr/cli/local_superlink_test.py | Asserts the disable-update-check env var is set when spawning the child process. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FLWR_DISABLE_UPDATE_CHECK = "FLWR_DISABLE_UPDATE_CHECK" | ||
| FLWR_UPDATE_CHECK_URL = f"{PLATFORM_API_URL}/update-check/flwr" | ||
| FLWR_UPDATE_CHECK_CONNECT_TIMEOUT_SECONDS = 1 | ||
| FLWR_UPDATE_CHECK_READ_TIMEOUT_SECONDS = 2 | ||
| FLWR_UPDATE_CHECK_CACHE_DIR = "cache" | ||
| FLWR_UPDATE_CHECK_CACHE_FILENAME = "update-check.json" | ||
| FLWR_UPDATE_CHECK_SHOW_INTERVAL_SECONDS = 24 * 60 * 60 |
There was a problem hiding this comment.
FLWR_DISABLE_UPDATE_CHECK/FLWR_UPDATE_CHECK_* are defined twice in this module (once starting at line 54 and again starting at line 63), with conflicting values (e.g., FLWR_UPDATE_CHECK_CACHE_DIR and FLWR_UPDATE_CHECK_SHOW_INTERVAL_SECONDS). In Python the later definitions silently override the earlier ones, which will break expectations/tests and make runtime behavior unclear. Please keep a single source of truth by removing one block and reconciling the intended values (and update any tests that rely on the chosen cache dir/interval).
| FLWR_DISABLE_UPDATE_CHECK = "FLWR_DISABLE_UPDATE_CHECK" | |
| FLWR_UPDATE_CHECK_URL = f"{PLATFORM_API_URL}/update-check/flwr" | |
| FLWR_UPDATE_CHECK_CONNECT_TIMEOUT_SECONDS = 1 | |
| FLWR_UPDATE_CHECK_READ_TIMEOUT_SECONDS = 2 | |
| FLWR_UPDATE_CHECK_CACHE_DIR = "cache" | |
| FLWR_UPDATE_CHECK_CACHE_FILENAME = "update-check.json" | |
| FLWR_UPDATE_CHECK_SHOW_INTERVAL_SECONDS = 24 * 60 * 60 |
framework/py/flwr/supercore/utils.py
Outdated
| def get_flwr_update_check_payload(process_name: str | None = None) -> dict[str, str]: | ||
| """Return the runtime payload sent to the update-check endpoint.""" | ||
| payload = { | ||
| "package_name": flwr_package_name, | ||
| "flwr_version": flwr_version, | ||
| "python_version": platform.python_version(), | ||
| "os": platform.system().lower(), | ||
| "os_version": platform.release(), | ||
| } | ||
| if process_name: | ||
| payload["process_name"] = process_name | ||
| return payload |
There was a problem hiding this comment.
The update-check implementation is being duplicated inside supercore/utils.py, but the repository already contains flwr/supercore/update_check.py with the same public API (get_flwr_update_check_payload, warn_if_flwr_update_available). Duplicating this logic in two places invites divergence/bugs (and you already have differing behaviors between the two versions). Consider re-exporting/importing from flwr.supercore.update_check instead of copying the implementation into utils, or remove the old module if the intent is to relocate it.
framework/py/flwr/supercore/utils.py
Outdated
| thread = threading.Thread( | ||
| target=_refresh_flwr_update_check_cache, | ||
| args=(process_name,), | ||
| daemon=False, |
There was a problem hiding this comment.
_start_flwr_update_check_refresh_thread creates a non-daemon thread (daemon=False). Non-daemon threads keep the interpreter alive, so short-lived CLI invocations (e.g., flwr --version) may block on the background network request despite being “in the background”. Setting this thread to daemon mode (or otherwise ensuring it can't delay process exit) would better match the intended startup-only/non-blocking check.
| daemon=False, | |
| daemon=True, |
framework/py/flwr/supercore/utils.py
Outdated
| def _should_refresh_flwr_update_check_cache(cache: dict[str, Any] | None) -> bool: | ||
| """Return True if cached state should be refreshed from the server.""" | ||
| if cache is None: | ||
| return True | ||
|
|
||
| last_checked_at = _parse_flwr_update_check_timestamp(cache.get("last_checked_at")) | ||
| if last_checked_at is None: | ||
| return True | ||
|
|
||
| return last_checked_at.date() < _get_utcnow().date() | ||
|
|
There was a problem hiding this comment.
_should_refresh_flwr_update_check_cache only considers the cached last_checked_at date. If the user upgrades/downgrades flwr on the same day, the cache will no longer match the current install (different package_name/flwr_version), but this function may still return False and skip refreshing until the next UTC day. Consider treating “cache does not match current install” as a reason to refresh immediately (similar to how _should_show_cached_flwr_update_message gates on _cache_matches_current_install).
| cache_dir = tmp_path / "cache" | ||
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
| (cache_dir / "update-check.json").write_text(json.dumps(body), encoding="utf-8") | ||
|
|
||
|
|
||
| def _read_update_check_cache(tmp_path: Path) -> dict[str, Any] | None: | ||
| cache_path = tmp_path / "cache" / "update-check.json" |
There was a problem hiding this comment.
The test helpers _write_update_check_cache/_read_update_check_cache hard-code the cache path as tmp_path / "cache" / "update-check.json", but production uses FLWR_UPDATE_CHECK_CACHE_DIR/FLWR_UPDATE_CHECK_CACHE_FILENAME from supercore.constant. This makes the tests brittle and can lead to false failures when the constants change (or when there are multiple definitions). Consider constructing the path from those constants, or use utils._get_flwr_update_check_cache_path() after patching get_flwr_home.
| cache_dir = tmp_path / "cache" | |
| cache_dir.mkdir(parents=True, exist_ok=True) | |
| (cache_dir / "update-check.json").write_text(json.dumps(body), encoding="utf-8") | |
| def _read_update_check_cache(tmp_path: Path) -> dict[str, Any] | None: | |
| cache_path = tmp_path / "cache" / "update-check.json" | |
| cache_path = utils._get_flwr_update_check_cache_path() | |
| cache_path.parent.mkdir(parents=True, exist_ok=True) | |
| cache_path.write_text(json.dumps(body), encoding="utf-8") | |
| def _read_update_check_cache(tmp_path: Path) -> dict[str, Any] | None: | |
| cache_path = utils._get_flwr_update_check_cache_path() |
This PR introduces a startup-only update check for
flwrcli