Add openlifu version check and installation button in login module#561
Conversation
ebrahimebrahim
left a comment
There was a problem hiding this comment.
This looks great to me! It works very well
I agree that we should check that nothing broke int he custom app before merging.
BTW did the login module always import openlifu on entry? That creates a situation where you can never validly get to the install button in the first place if you didn't already have some version of openlifu installed.
| def get_required_openlifu_version() -> "Optional[str]": | ||
| """Return the required openlifu version pinned in | ||
| python-requirements.txt, or None.""" | ||
| requirements_path = Path(__file__).parent / 'Resources/python-requirements.txt' | ||
| for line in requirements_path.read_text().splitlines(): | ||
| if line.startswith('openlifu=='): | ||
| return line.split('==', 1)[1].strip() | ||
| return None |
There was a problem hiding this comment.
I am thinking about what would happen when someone puts a git+... sort of requirement for openlifu, as we often do while testing unreleased versions of openlifu. I think it's okay to not support version checking in those cases, since they are debug cases, but should the version check auto-pass or auto-fail? Right now I think it autopasses because this would return None. Maybe that is fine, just brinign up the question.
There was a problem hiding this comment.
Good point - I didn't consder that situation. You're right, right now it auto-passes so the user isn't notified that a dev version needs to be installed. Currently the version check only fails if openlifu is not installed or it is the wrong released version.
I'm going to push a commit that handles the dev situation such that it auto-fails/notifies the user - it's an easy check and doesn't break anything else.

While testing this, I noticed that there is potentially an issue in the current main branch of openlifu (OpenwaterHealth/openlifu-python@9f17b94). When installed, openlifu.util.assets is not recognized. But I didn't get this error with an older commit (pre v0.18). Do you mind testing it on your end?
Below is a suggested fix from a claude conversion, which I think I agree with but see if you think there's an issue ... there are few cases to consider: no openlifu, openlifu but no database, database but no users in it, .... We can do it in this PR or we can do it later, all fine. (ai generated content)Here's the issue and proposed fix: Problem: Proposed fix: Lazy-init the default users. Skip def _ensureDefaultUsers(self):
if self._default_anonymous_user is None and python_requirements_exist():
self._initDefaultUsers()Call if python_requirements_exist():
self._initDefaultUsers()
self.logic.active_user = self._default_anonymous_userThis is safe because before openlifu is installed, there's no database and no login — the default users serve no purpose. Once the user installs openlifu via Home and navigates back or connects a database, For the custom app, openlifu is always bundled so this path doesn't apply. If desired, the custom app could add an early fatal error if openlifu isn't importable, rather than silently breaking. |
Yes, agreed, a separate PR. We can put it off indefinitely for now ... it is good that we have a clear explanation of the problem and possible solutions recorded here |
ebrahimebrahim
left a comment
There was a problem hiding this comment.
Looks great! One small change and we should be good to merge
| return False | ||
| required_hash = required.split('+g')[-1] | ||
| installed_hash = installed.split('+g')[-1] | ||
| return installed_hash.startswith(required_hash) |
There was a problem hiding this comment.
| return installed_hash.startswith(required_hash) | |
| return required_hash.startswith(installed_hash) or installed_hash.startswith(required_hash) |
One might be shorter than the other which would make startswith fail; a symmetric comparison like this removes the worry
(commit hashes get truncated to various lengths by various tools/conventions)
6eeeeac to
ab7d49b
Compare


Closes #544
Todo: