Skip to content
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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

goanpeca
Copy link
Collaborator

@goanpeca goanpeca commented Nov 29, 2022

Split from #34

Overview

Bundle Application and Constructor Manager

Constructor based installers

These installers will contain 3 main environments when created:

  • A base environment which will contain:
    • conda (package and environment manager)
    • mamba (conda "replacement" for performing faster solves, transactions and downloads)
    • conda-lock (create locked and reproducible environments)
  • A constructor-manager environment which will contain:
  • An application environment, using the conventions <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 the constructor-manager-backend. It provides a python library to run workers and query information on the different processes managed by the constructor manager

The 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 the conda-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_manager: open the constructor manager ui for a given package.
  • open_application: open a give application created with conda and menuinst un a cross platform way

Some 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

  • Tests
  • Release package to pypi
  • Release package to conda-forge

Future work

  • provide non-qt based worker workflows (perhaps use async or psygnal?). Currently tied to qt because napari uses that, but should not rely on it necessarily
  • provide uninstall method
  • provide get status method of a current command execution (update, revert, reset, restore, lock)

@goanpeca goanpeca self-assigned this Nov 29, 2022
@goanpeca goanpeca changed the title Add constructor manager CLI Add constructor manager api Nov 29, 2022
@goanpeca goanpeca changed the title Add constructor manager api Add constructor manager client Feb 15, 2023
@goanpeca goanpeca changed the title Add constructor manager client Add constructor manager api Mar 8, 2023
@goanpeca goanpeca requested review from dalthviz and liu-ziyang March 13, 2023 12:25
@goanpeca goanpeca marked this pull request as ready for review March 13, 2023 12:29
@goanpeca
Copy link
Collaborator Author

Fixing the tests, but this one is ready for review @aganders3, @dalthviz, @potating-potato

Thanks!

Copy link
Member

@dalthviz dalthviz left a 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
Copy link
Member

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?

Copy link
Collaborator Author

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 ?

Copy link

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
Copy link
Member

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'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

print(result)


worker = check_updates(package_name="napari", current_version="0.4.10", channel="conda-forge")
Copy link
Member

@dalthviz dalthviz Mar 13, 2023

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:

Suggested change
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"])

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

## Usage

```python
from constructor_manager.api import check_updates
Copy link
Member

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:

Suggested change
from constructor_manager.api import check_updates
from constructor_manager_api.api import check_updates

Copy link
Collaborator Author

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 :)

Comment on lines 177 to 191
"""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.
"""
Copy link
Member

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Comment on lines 156 to 168
"""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.
"""
Copy link
Member

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)

Comment on lines +22 to +35
# 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()
Copy link
Member

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?

Copy link
Collaborator Author

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!

Copy link
Contributor

@aganders3 aganders3 left a 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.

Comment on lines +105 to +106
detached = cmd != "status"
detached = False
Copy link
Contributor

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?

Copy link
Collaborator Author

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():
Copy link
Contributor

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.

Copy link
Collaborator Author

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)
Copy link
Contributor

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)

Copy link
Collaborator Author

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!

Comment on lines +35 to +36
if not self._program.is_file():
raise FileNotFoundError(f"Could not find {self._program}")
Copy link
Contributor

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.

Copy link
Collaborator Author

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
Copy link
Member

@dalthviz dalthviz Mar 14, 2023

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

Copy link
Collaborator Author

@goanpeca goanpeca Mar 15, 2023

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking it appears in my env as an .exe:

imagen

I think the .bat executables exist but for conda inside the condabin directory:

imagen

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants