-
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
Add constructor manager api #46
base: main
Are you sure you want to change the base?
Conversation
8eab40e
to
2f76bcd
Compare
Fixing the tests, but this one is ready for review @aganders3, @dalthviz, @potating-potato 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 an initial review with a couple of suggestions to update the README description, some comments regarding docstrings and maybe a missing dependency (pyyaml
) and package metadata (autor-email
).
I will try later to setup the constructor-manager-cli
from #34 to see if I can run the functions (like check_updates
for example). But besides the comments left this LGTM 👍
author_email = napari-steering-council@googlegroups.com | ||
url = https://github.com/napari/packaging/constructor-manager-api | ||
author = napari | ||
author_email = TODO |
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 there is not a email to use here, 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.
Not sure what email we should use 🤔
@kephale do you have a suggestion ?
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.
napari-steering-council@googlegroups.com is still the official napari SC email address, this can be reverted.
@@ -23,7 +23,7 @@ classifiers = | |||
Topic :: Scientific/Engineering :: Image Processing | |||
project_urls = | |||
Bug Tracker = https://github.com/napari/packaging/issues | |||
Source Code = https://github.com/napari/packaging/constructor-manager | |||
Source Code = https://github.com/napari/packaging/constructor-manager-api |
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 pyyaml
be added in the install_requires
section of this package? I tried doing:
from constructor_manager_api.api import check_updates
and I got an error related to the import yaml
done for the settings module:
>>> from constructor_manager_api.api import check_updates
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "E:\Acer\Documentos\Quansight\Napari\packaging-otros\gonzalo\packaging\constructor-manager-api\src\constructor_manager_api\api.py", line 10, in <module>
from constructor_manager_api.utils.settings import save_settings
File "E:\Acer\Documentos\Quansight\Napari\packaging-otros\gonzalo\packaging\constructor-manager-api\src\constructor_manager_api\utils\settings.py", line 5, in <module>
import yaml
ModuleNotFoundError: No module named 'yaml'
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 you are 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.
Fixed
constructor-manager-api/README.md
Outdated
print(result) | ||
|
||
|
||
worker = check_updates(package_name="napari", current_version="0.4.10", channel="conda-forge") |
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.
Checking the check_updates
function signature I think this should be something like:
worker = check_updates(package_name="napari", current_version="0.4.10", channel="conda-forge") | |
worker = check_updates(package_name="napari", current_version="0.4.10", channels=["conda-forge"]) |
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.
Fixed
constructor-manager-api/README.md
Outdated
## Usage | ||
|
||
```python | ||
from constructor_manager.api import check_updates |
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.
With the directory structure change I think this should be something like:
from constructor_manager.api import check_updates | |
from constructor_manager_api.api import check_updates |
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, but I think I prefer adding those imports imports on the init so there is no need for the second api submodule (which looks weird :)
"""Check for updates. | ||
|
||
Parameters | ||
---------- | ||
package_name : str | ||
Name of the package to check for updates. | ||
version : str | ||
Version of package to execute action on, by default ``None``. | ||
|
||
Returns | ||
------- | ||
ConstructorManagerWorker | ||
Worker to check for updates. Includes a finished signal that returns | ||
a ``dict`` with the result. | ||
""" |
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.
Seems like this docstring needs some changes (missing plugins_url
kwarg docstring, main docstring refers to check for updates instead of package)
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.
Fixed!
"""Check for updates. | ||
|
||
Parameters | ||
---------- | ||
package_name : str | ||
Name of the package to check for updates. | ||
|
||
Returns | ||
------- | ||
ConstructorManagerWorker | ||
Worker to check for updates. Includes a finished signal that returns | ||
a ``dict`` with the result. | ||
""" |
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.
Seems like this docstring needs some changes (description refers to check for updates instead of version)
# worker = check_updates( | ||
# "napari", | ||
# current_version="0.4.15", | ||
# channel="napari", | ||
# dev=True, | ||
# ) | ||
# worker = check_updates( | ||
# "napari", | ||
# build_string="pyside", | ||
# plugins_url="https://api.napari-hub.org/plugins", | ||
# ) | ||
# worker = check_version("napari") | ||
# worker.finished.connect(_finished) | ||
# worker.start() |
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.
Maybe this code could be added as part of the README as example usage of the package?
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 that was the plan, will move it there!
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.
Sorry I haven't had a chance to actually run this yet. It looks good so far, just a few nitpicks.
detached = cmd != "status" | ||
detached = False |
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.
ActionsEnum
does not have a "status"
member, but I would also compare directly with the enum instead of a raw string here. Though this is hard-coded on the next line anyway so maybe this is just old debugging code left in?
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.
Whoops., you are right, fixing!
if (current / "envs").exists() and (current / "envs").is_dir(): | ||
return current | ||
|
||
if current.parent.name == "envs" and current.parent.is_dir(): |
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 also check if the path exists (as above)? It might be enough for each to just check is_dir()
as I don't think it just returns False
if the path doesn't exist.
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.
Good point!
loaded_settings = _default_settings.copy() | ||
if path.exists(): | ||
with open(path) as f: | ||
data = yaml.load(f, Loader=yaml.FullLoader) |
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.
Maybe not a huge deal, but could this instead use SafeLoader
? (or I think just safe_load
will do the same thing)
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, it is better. Will update!
if not self._program.is_file(): | ||
raise FileNotFoundError(f"Could not find {self._program}") |
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 don't think this is necessary as _executable()
looks like it will already raise such an exception.
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.
True
, code refactor that left in there 🤦🏼
Thanks!
bin = "constructor-manager-cli" | ||
envs = ["_constructor-manager", "constructor-manager", "base"] | ||
for env in envs: | ||
program = get_prefix_by_name(env) / "bin" / bin |
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.
Was checking on Windows and I think there instead of bin
the directory where executables are stored is Scripts
and the executable has the .exe
extension so for Windows probably this should be something like:
bin = "constructor-manager-cli.exe"
program = get_prefix_by_name(env) / "Scripts" / bin
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 @dalthviz, indeed, I ran into the same issue. Thanks for checking!, although it should be .bat
the extension, no?
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.
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.
On Windows entry points use a C launcher that mimics what a shebang would do.
constructor-manager-cli.exe
will find:
- constructor-manager-cli-script.py` next to itself
- a
python.exe
interpreter nearby
After that it will run python.exe constructor-manager-cli-script.py
.
So yes, it's an .exe
in Scripts
. I think a subprocess without the .exe
extension still finds the required executable, but I am not sure. Better be explicit.
Split from #34
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
(Add constructor manager cli #34)constructor-manager-api
(this PR)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-api
This pull request is in charge of creating the
constructor-manager-api
package which in its present form is a qtpy (worker and signal) based library providing an API to call theconstructor-manager-backend
. It provides a python library to run workers and query information on the different processes managed by the constructor managerThe library is imported with
from constructor_manager_api import check_updates, update, revert, restore, open_application
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.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)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>
.lock-environment
: create a lock file of the current state of the application environment. This can be called by the applications (using constructor-manager-api) to check for changes in the environment. If no changes are detected, no lock is created.open_application
: open a give application created withconda
andmenuinst
un a cross platform waySome of these commands can be run in parallel others create a lock to prevent multiple instances of an update/restore/revert process.
This library is meant to be used in the application environment by the applications themselves so they can query the information to know about new updates and either trigger them directly in a detached process (to be implemented in a following version) or to open the
constructor-manager-ui
to handle those actions directly.More information on the README of the package
Work in progress
Future work