-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update UI and connect to constructor manager API #63
base: main
Are you sure you want to change the base?
Conversation
b36d81c
to
0097100
Compare
Fixing the tests, but this one is ready for review @aganders3, @dalthviz, @potating-potato (Also need to complete docstrings) Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @goanpeca ! Left some comments/questions regarding commented out code, some qss styles, some print
statements, the CI workflow reorganization and a traceback related with the log level setting that appeared when I was trying to run some commands
|
||
return install_information_group | ||
log_level = getattr(logging, log_level.upper()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried running constructor-manager-ui napari --current-version 0.4.16 --build-string pyside --plugins-url https://api.napari-hub.org/plugins --channel conda-forge
locally and got the following traceback related to this line:
Traceback (most recent call last):
File "C:\Users\dalth\anaconda3\envs\constructor-manager\lib\runpy.py", line 196, in _run_module_as_main
return _run_code(code, main_globals, None,
File "C:\Users\dalth\anaconda3\envs\constructor-manager\lib\runpy.py", line 86, in _run_code
exec(code, run_globals)
File "C:\Users\dalth\anaconda3\envs\constructor-manager\Scripts\constructor-manager-ui.exe\__main__.py", line 7, in <module>
File "E:\Acer\Documentos\Quansight\Napari\packaging\constructor-manager-ui\src\constructor_manager_ui\main.py", line 58, in run
_configure_logging(settings["log"])
File "E:\Acer\Documentos\Quansight\Napari\packaging\constructor-manager-ui\src\constructor_manager_ui\main.py", line 33, in _configure_logging
log_level = getattr(logging, log_level.upper())
AttributeError: 'NoneType' object has no attribute 'upper'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the issue of calling on windows. Fixing over the CLI PR
background-color: @background-color13; | ||
background-color: @background-selection-color01; | ||
background-color: #758193; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should here be only the entry for #758193
while also making it part of the colors constants available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can do that
# f"Last modified {self.current_version['last_modified']}" | ||
# ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is part of a TODO (as well as the commented text for labels code bellow), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
# Installation Actions | ||
feedback_group = self._create_feedback_group() | ||
main_layout.addWidget(feedback_group, stretch=1) | ||
print(self.log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this and other print
statements in the file be transformed into logger.info
/logger.debug
calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes absolutely
# while ConstructorManagerWorker._WORKERS: | ||
# worker = ConstructorManagerWorker._WORKERS.pop() | ||
# self._terminate_worker(worker) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed or uncommented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, we do not want to force close in the middle of an update.
- name: Test constructor-manager-ui (linux) | ||
if: contains(matrix.platform, 'ubuntu') | ||
run: | | ||
# Run Tests | ||
cd constructor-manager-ui/src | ||
xvfb-run pytest constructor_manager_ui --cov=constructor_manager_ui | ||
|
||
- name: Test constructor-manager-ui (other) | ||
if: "!contains(matrix.platform, 'ubuntu')" | ||
run: | | ||
# Run Tests | ||
cd constructor-manager-ui/src | ||
pytest constructor_manager_ui --cov=constructor_manager_ui |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the UI part will not use tox
for the tests on the CI? Should the tox.ini
file be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since all the workflow for these packages revolves around constructor using a base environment and a constructor-manager environment, tox does not help a lot. Will remove!
Hi @goanpeca I'm curious whether you have examples of built Otherwise as I understand it each piece can be played with individually either from the command line or by following the instructions in the PR description? |
Hi @DragaDoncila, I will update a small snippet to install all the needed deps (including installing from these PRs) so you can test the whole workflow. Thanks! |
Overview
## Constructor based installers
These installers will contain 3 main environments when created:
base
environment which will contain:constructor-manager
environment which will contain:constructor-manager
constructor-manager-api
(Add constructor manager api #46)constructor-manager-ui
(Update UI and connect to constructor manager API #63)<package-name>-<version>
, for the example image above,napari-0.4.15
More information on packaging on https://napari.org/dev/developers/packaging.html#constructor-based-installers
constructor-manager-ui
This pull request is in charge of connecting the previously created UI (#40) the
constructor-manager-ui
package which in its present form is a pyqt/pyside based application. It provides a graphical interface to run the different actions to manage an application built withconstructor
The ui is called from the command line interface with
constructor-manager-ui <package-name>
. Since the application will be called using standard application calls created by menuinst, arguments will be passed with configuration files handled byconstructor-manager-client
calls.check-updates
: Query for new updates available for the managed application by providing one or more anaconda.org channels. By default we query theconda-forge
channel located at https://anaconda.org/conda-forge/update
: update a current application to a new version of it. This process will create a new conda environment following the convention<package-name>-<new-version>
, create new menu shortcuts (using the menuinst branch https://github.com/conda/menuinst/tree/cep-devel/menuinst), create a new restore point (using conda-lock), remove the old environment and the corresponding shortcuts for the old versions of the managed application.revert
: this will revert the current application to the previously installed version on the computer if a restore point is found. This will follow a similar process of creating a new conda environment with the convention<package-name>-<old-version>
.restore
: similar to revert but restore to a previously found state of the current version. This command could become a single one by providing the specific restore file (which is created by conda lock)open
: open the managed give application.Work in progress
Future work