FIREFLY-1331: Add Version validation for compatibility check between python client and firefly server#79
FIREFLY-1331: Add Version validation for compatibility check between python client and firefly server#79jaladh-singhal wants to merge 4 commits intomasterfrom
Conversation
- Introduced _server_compat.py for managing server version compatibility. - Implemented version retrieval and compatibility checking in FireflyClient. - Updated pyproject.toml to include packaging dependency.
… validation logic
And update documentation and pyproject.toml for pytest
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
jaladh-singhal
changed the title
|
|
||
| [project.optional-dependencies] | ||
| tests = [ | ||
| "pytest", |
There was a problem hiding this comment.
this is an optional dependency for tests so won't impact end user.
| "websocket-client", | ||
| "requests" | ||
| "requests", | ||
| "packaging", |
There was a problem hiding this comment.
packaging is one of the core utilities developed and maintained by python packaging authority (pypa): https://github.com/pypa/packaging. It's also used by pip itself internally so in most cases, it is already installed in user's python environment.
| def is_server_compatible(server_version: str|None) -> bool: | ||
| if not server_version: | ||
| return False | ||
| version = standardize_version(server_version) | ||
| return version is not None and version >= _MIN_VERSION |
There was a problem hiding this comment.
I am concerned that this might not be the best way to validate the version. This look like just a string compare. I think you need to separate the major and the minor the do something similar to BrowserInfo.js. That way you can ignore the type of build.
There was a problem hiding this comment.
It's not a string compare, it's comparing two packaging.Version objects (standardize_version converts firefly version string to Version object) which knows how to handle relational operators.
You can look at the unit tests to confirm if it's missing any of the cases you are concerned about: https://github.com/Caltech-IPAC/firefly_client/blob/FIREFLY-1331-version-validation/tests/test_server_compat.py#L31-L49
There was a problem hiding this comment.
ok, I am going to probably need an overview of how this works. Maybe we can do that on Monday.
There was a problem hiding this comment.
You can read each test case as: ver >= min_ver = bool. So:
_FIXED_MIN_VERSION = standardize_version('2026.1-DEV')
('2026.1-PRE-3', True), # pre-release of same version cycle
('2026.1', True), # formal release >= minimumbecomes:
'2026.1-PRE-3' >= '2026.1-DEV' = True
'2026.1' >= '2026.1-DEV' = True
and so on
There was a problem hiding this comment.
Yes, I can demo it on Monday too.
2 participants
Fixes FIREFLY-1331
FireflyClientinitialization — raisesValueErrorif the server version is below the required minimum, warns and proceeds if the version endpoint is unreachable_server_compat.pymodule with version parsing logic, which also handles Firefly's non-standard version strings (DEV with branch/commit suffix, PRE releases)tests/test_server_compat.py) and a dev test notebook (examples/development_tests/test-version.ipynb)Also see Firefly PR: Caltech-IPAC/firefly#1936
Extra:
pytestas an optional[tests]dependency; updated developer docs with instructions to run testsurl_cmd_servicefromsticky/CmdSrvtoCmdSrv/syncwhich seems more modernTesting
examples/development_tests/test-version.ipynb): nb-review's rendered version in the following comment.tests/test_server_compat.py) to understand how version parsing and compatiblity check logic is working for different cases