Fix blockers system - #1513
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1513 +/- ##
==========================================
- Coverage 75.35% 75.26% -0.10%
==========================================
Files 115 115
Lines 7378 7381 +3
==========================================
- Hits 5560 5555 -5
- Misses 1818 1826 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
548b751 to
83e8834
Compare
| include = True # build-in panel | ||
|
|
||
| def update(self, specific=""): | ||
| def _update(self, specific=""): |
There was a problem hiding this comment.
This is a breaking change that has already been addressed in the only plugin that used it (aiidaplugins/aiida-qe-xspec#37)
There was a problem hiding this comment.
If this method is being used in the plugins, why make it private?
| self._toggle_eigenvalues_widget() | ||
|
|
||
| def _update(self): | ||
| def _update_ui(self): |
There was a problem hiding this comment.
Same comment as above
03c071d to
c925406
Compare
| if self.is_blocked: | ||
| self.state = State.BLOCKED |
There was a problem hiding this comment.
Blocking a step is now an official state we can react to. We use this to also change the color of a blocked step to red (same as failed) to better alert the user of a blocking issue.
| else: | ||
| info.value = "" | ||
| model.update() | ||
| panel.refresh() |
There was a problem hiding this comment.
Instead of just triggering a model update, we trigger a full panel refresh, which unlinks any widgets before updating the model, so that in case a model update triggers a UI update, we don't end up with orphan links.
| tl.dlink( | ||
| (model, "blockers"), | ||
| (self, "blockers"), | ||
| ) |
There was a problem hiding this comment.
HasBlockers is now placed on the root Model, so any Model can be blocked. Hence, there is no longer a need to check this on a given model.
|
|
||
| def update_blockers(self): | ||
| blockers = list(self._check_blockers()) | ||
| blockers = list(self._check_blockers() or []) |
There was a problem hiding this comment.
_check_blockers yields str blockers, or None if no blockers are yielded. The latter scenario cannot be used in list, so we guard here with or []. list([]) is just [] 👍
| def reset_blockers(self): | ||
| self.blockers = [] | ||
| if isinstance(self, HasModels): | ||
| for _, model in self.get_models(): | ||
| if isinstance(model, HasBlockers): | ||
| model.reset_blockers() |
There was a problem hiding this comment.
In general good to have a reset for the blockers register, but more practically, this is used when refreshing panels.
| class Model( | ||
| tl.HasTraits, | ||
| HasBlockers, | ||
| metaclass=MetaHasTraitsLast, | ||
| ): |
There was a problem hiding this comment.
Extending the blocking mecahnism to all models!
| `updated` : `bool` | ||
| Whether the model has been updated. |
There was a problem hiding this comment.
Unused, but also overcomplicates things at minimal-to-no gain.
| if self.locked or specific == "widgets": | ||
| return | ||
| self._update(specific) | ||
| self.update_blockers() | ||
| self.update_state() | ||
|
|
||
| def update_state(self): | ||
| """Update the model state.""" |
There was a problem hiding this comment.
Generalizing the concept of refreshing a model at the root Model level.
| def _update(self, specific=""): | ||
| """Internal method to update the model.""" | ||
| pass |
There was a problem hiding this comment.
Within the generalization, this allows each specific Model subclass to define what it means by "updating", in addition to the general part.
| super().__init__(*args, **kwargs) | ||
|
|
||
|
|
||
| CSM = t.TypeVar("CSM", bound=ConfigurationSettingsModel) |
There was a problem hiding this comment.
Missed this in a previous PR deprecating ConfigurationSettingsModel
| def update_blockers(self): | ||
| if self.include: | ||
| super().update_blockers() |
There was a problem hiding this comment.
For PanelModel instances, we want to update the blocking system ONLY if the panel is included.
|
|
||
| [tool.ruff.lint] | ||
| ignore = ["E501", "E402", "TRY003", "RUF012", "N806"] | ||
| ignore = ["ARG002", "E501", "E402", "TRY003", "RUF012", "N806"] |
There was a problem hiding this comment.
ARG002 to avoid ruff complaining about the often unused specific argument of update
|
@AndresOrtegaGuerrero just finished a self-review. Hope it helps you review this medium PR 🙂 Happy to answer any questions 🙏 |
danielhollas
left a comment
There was a problem hiding this comment.
Just had a quick look, seems reasonable, but I am not really familiar with the current QeApp codebase.
yakutovicha
left a comment
There was a problem hiding this comment.
We discussed the PR in a call, the changes make sense to me.
To prevent users from submitting calculations with unconfigured codes, we filter them out. However, this SHOULD NOT happen when loading a process, since the user cannot submit in that state. But it was, so step 3 was emitting blockers due to unselected codes. Prior to #1513, the blockers didn't do their job of blocking the step, but now they do. So if someone loads the app from a process that was run by someone else (e.g., a downloaded example), step 3 is blocked and step 4 never loads. This PR adds a `filter_codes_for_user` flag injected through the resources refresh mechanism that is generally `True` but set to `False` if we're loading a process. In addition, the PR relaxes default code handling, falling back on the first available code option if the default code does not exist. Finally, the PR removes the blocking of step 3 w.r.t the local QE installation status, since users should still be allowed to proceed with remote submission.
This PR fixes various issues with the blockers system. It also extends the mechanism to all models, for furture support.
In passing, the PR fixes a few inconsistencies with state handling and component updating.
For context, a blocking system was introduced a while back to the app to allow blocking wizard steps for any given reason. For example, the submission step is blocked if QE or its codes are still being installed. Since, the system was expanded in the configuration step to block for unreasonable configuration. This is implemented in a way that allows each panel model to define its own blocker(s). However, the system was incomplete, evident by various inconsistencies and bugs discovered along the way. This PR aims to resolve these issues, as well as introduce
BLOCKEDas a new actionable wizard step state, so that we can better track blocking events.