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 cli #34

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

Conversation

goanpeca
Copy link
Collaborator

@goanpeca goanpeca commented Oct 5, 2022

Related to #10

Overview

Screenshot 2023-02-15 at 10 16 33 AM

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-backend

This pull request is in charge of creating the constructor-manager-backend package which in its present form is a CLI utility that will work under the hood with conda, mamba and conda-lock (living in the base environment) to provide the following functionalities in the creation of bundle applications:

The backend can be called using the CLI program constructor-manager and the following actions are available

  • 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.
  • 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)
  • lock-environment: create a lock file of the current state of the application environment. This can be called by the client applications (using constructor-manager-client) to check for changes in the environment. If no changes are detected, no lock is created.
  • uninstall: to be implemented
  • get-status: get information on a currently running update/restore/revert in progress.

Some of these commands can be run in parallel others create a lock to prevent multiple instances of an update/restore/revert process.

More information on the README of the package

Work progress

  • Installer Manager
  • Action Manager
    • check-updates
    • update
    • restore
    • revert
    • lock-environment
    • get-status (could be deferred for the next iteration)
    • uninstall (mostly dependent on the constructor stack, but could be implemented partially here as well)
  • Utilities
  • Request handling
  • Testing
    • Action Manager (partial testing in place)
    • Installer Manager (partial testing in place)
    • Utilities
    • Request handling
  • Release package to pypi
  • Release package to conda-forge

Future work

  • Handle multiple applications
  • Provide rich information for debugging
  • Provide background update within the running application

@goanpeca goanpeca mentioned this pull request Oct 5, 2022
14 tasks
@goanpeca goanpeca changed the title Add initial structure for backend constructor updater Add initial structure for backend constructor manager Oct 26, 2022
@goanpeca goanpeca marked this pull request as ready for review October 26, 2022 21:46
@goanpeca goanpeca marked this pull request as draft November 29, 2022 03:26
@goanpeca goanpeca mentioned this pull request Nov 29, 2022
6 tasks
@goanpeca goanpeca changed the title Add initial structure for backend constructor manager Add constructor manager backend cli Nov 29, 2022
@goanpeca goanpeca self-assigned this Nov 29, 2022
@goanpeca goanpeca force-pushed the constructor-updater branch 2 times, most recently from 778fdf9 to d4e630a Compare November 30, 2022 01:50
@goanpeca goanpeca changed the title Add constructor manager backend cli Add constructor manager backend Feb 15, 2023
@goanpeca goanpeca changed the title Add constructor manager backend Add constructor manager cli Mar 8, 2023
@goanpeca
Copy link
Collaborator Author

@dalthviz, @aganders3 this one is ready for review, except for the file insttaller.oy which I am simplifying a lot since we do not need to use a queue and the other abtsractions.

(also fixing the windows test)

Thanks 🚀

@goanpeca goanpeca marked this pull request as ready for review March 22, 2023 15:02
@goanpeca goanpeca requested a review from dalthviz March 22, 2023 15:03
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 some initial comments/questions mostly on the README file, the menuinst dependency and some commented out code that I found lying around. Tried checking some of the commands but I got some tracebacks with the check-updates one after trying to setup a napari installation to be detected (before that the command was working and it detected I didn't have napari installed 👍). Maybe I made some mistake trying to setup things 😅 but I left a comment just in case with some initial debugging I did.

Comment on lines 35 to 39
# - repo: https://github.com/pycqa/isort
# rev: 5.11.5
# hooks:
# - id: isort
# name: isort (python)
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 could be removed?

Copy link
Contributor

@aganders3 aganders3 Mar 23, 2023

Choose a reason for hiding this comment

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

Yes isort can be replaced by ruff --fix with the I rules configured, but I don't think they're part of the defaults.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing

- mamba
- packaging
- requests
- pyyaml
Copy link
Member

Choose a reason for hiding this comment

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

Should menuinst be listed as a requirement here too?

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 we are relying on a fork so in the meantime it should install that fork. Will add a line

Comment on lines 459 to 463
### Menuinst

```bash
pip install git+https://github.com/conda/menuinst.git@cep-devel
```
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 could be better at the top of the README with the rest of the instructions to install?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Check for the version of the currently (latest?) installed package.

```bash
constructor-manager check-version "napari"
Copy link
Member

Choose a reason for hiding this comment

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

I think this and the commands bellow need to use constructor-manager-cli instead of constructor-manager since that's how the entrypoint is still declared at setup.cfg or maybe setup.cfg needs to be updated?

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


__version__ = "0.0.1"
__version__ = "0.1.0"
VERSION_INFO = (0, 1, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Is this VERSION_INFO constant needed?

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 really I think 🤔

result = []
if args.command in _COMMANDS:
result = method(**method_kwargs)
# time.sleep(5)
Copy link
Member

@dalthviz dalthviz Mar 22, 2023

Choose a reason for hiding this comment

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

Maybe this and the comment below related to time.sleep should be removed?

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 for testing the lock file


packages_original_format = self._installer.list(str(prefix), block=True)
for pkg in packages_original_format: # type: ignore
source = "pip" if pkg["platform"] == "pypi" else "conda"
Copy link
Member

Choose a reason for hiding this comment

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

After setting up an env with

conda create -n napari-0.4.15 "napari=0.4.15" "napari-menu=0.4.15" affinder -c conda-forge -y --override-channels

And adding to the conda-meta dir inside the env a .napari_is_bundled_constructor, I tried running:

constructor-manager-cli check-packages "napari=0.4.15" --plugins-url https://api.napari-hub.org/plugins

I got a response error:

{
    "data": {},
    "error": "Traceback (most recent call last):\n  File \"E:\\Acer\\Documentos\\Quansight\\Napari\\packaging-otros\\gonzalo\\packaging\\constructor-manager\\src\\constructor_manager\\main.py\", line 116, in main\n    result = _execute(args, lock_file_path)\n  File \"E:\\Acer\\Documentos\\Quansight\\Napari\\packaging-otros\\gonzalo\\packaging\\constructor-manager\\src\\constructor_manager\\main.py\", line 65, in _execute\n    result = method(**method_kwargs)\n  File \"E:\\Acer\\Documentos\\Quansight\\Napari\\packaging-otros\\gonzalo\\packaging\\constructor-manager\\src\\constructor_manager\\actions.py\", line 561, in check_packages\n    packages, _ = self._get_installed_packages(prefix, plugins_url)\n  File \"E:\\Acer\\Documentos\\Quansight\\Napari\\packaging-otros\\gonzalo\\packaging\\constructor-manager\\src\\constructor_manager\\actions.py\", line 125, in _get_installed_packages\n    source = \"pip\" if pkg[\"platform\"] == \"pypi\" else \"conda\"\nTypeError: string indices must be integers\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"E:\\Acer\\Documentos\\Quansight\\Napari\\packaging-otros\\gonzalo\\packaging\\constructor-manager\\src\\constructor_manager\\main.py\", line 121, in main\n    sys.stdout.write(json.dumps(data, indent=4))\n  File \"C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\lib\\json\\__init__.py\", line 238, in dumps\n    **kw).encode(obj)\n  File \"C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\lib\\json\\encoder.py\", line 201, in encode\n    chunks = list(chunks)\n  File \"C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\lib\\json\\encoder.py\", line 431, in _iterencode\n    yield from _iterencode_dict(o, _current_indent_level)\n  File \"C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\lib\\json\\encoder.py\", line 405, in _iterencode_dict\n    yield from chunks\n  File \"C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\lib\\json\\encoder.py\", line 438, in _iterencode\n    o = _default(o)\n  File \"C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\lib\\site-packages\\conda\\__init__.py\", line 145, in _default\n    return getattr(obj.__class__, \"to_json\", _default.default)(obj)\n  File \"C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\lib\\json\\encoder.py\", line 179, in default\n    raise TypeError(f'Object of type {o.__class__.__name__} '\nTypeError: Object of type TypeError is not JSON serializable\n"
}

Maybe I'm not correctly setting up the envs but just in case sharing the response. The error seems related with the CondaInstaller list call (self._installer.list) returning {'stdout': ''}. Checked that method and seems like since it runs the list command with mamba it doesn't work if you have installed menuinst from the cep-devel branch. Running mamba list --prefix C:\\Users\\dalth\\anaconda3\\envs\\constructor-manager\\envs\\napari-0.4.15 --json causes:

Traceback (most recent call last):
  File "C:\Users\dalth\anaconda3\envs\constructor-manager\Scripts\mamba-script.py", line 6, in <module>
    from mamba.mamba import main
  File "C:\Users\dalth\anaconda3\envs\constructor-manager\lib\site-packages\mamba\mamba.py", line 53, in <module>
    from mamba.mamba_shell_init import shell_init
  File "C:\Users\dalth\anaconda3\envs\constructor-manager\lib\site-packages\mamba\mamba_shell_init.py", line 8, in <module>
    from conda.core.initialize import initialize, initialize_dev, make_initialize_plan
  File "C:\Users\dalth\anaconda3\envs\constructor-manager\lib\site-packages\conda\core\initialize.py", line 71, in <module>
    from menuinst.knownfolders import get_folder_path, FOLDERID
ModuleNotFoundError: No module named 'menuinst.knownfolders'

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, I had not tested on woindows so the command is not working. Testing on windows

constructor_manager_dir.mkdir(parents=True, exist_ok=True)
lock_file_path = constructor_manager_dir / "constructor-manager.lock"

# result = _execute(args, lock_file_path)
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 should be removed?

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


@lru_cache
def get_config_path() -> Path:
# path = get_prefix_by_name("base") / "var" / "constructor-manager"
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 should be removed?

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 +194 to +195
# if not is_spyder_process(int(pid)):
# raise(OSError(errno.ESRCH, 'No such process'))
Copy link
Member

Choose a reason for hiding this comment

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

Probably this should be removed or updated to detect a napari related process?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a note

Comment on lines 48 to 49
constructor-manager check-updates "napari=0.4.16=*pyside*" -c conda-forge
constructor-manager check-updates "napari=*=*pyside*" -c conda-forge
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a note (or link?) about the format of this version spec. I was not familiar with the syntax for specifying build strings and wildcards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding extra info

We use a subset of the version spec defined by [conda](https://docs.conda.io/projects/conda-build/en/latest/resources/package-spec.html)

Namely the spec coontains a `<package-name>=<version>=<build-string>` where the `*` symbol can be used as a wildcard.

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'm still going through this, and still haven't run it yet. This looks great so far!

I mostly stopped reviewing when I got to the tests for now, but I'll check those out after I get it installed and do some manual testing.

Comment on lines 16 to 37
def is_stable_version(version: Union[Tuple[str, ...], str]) -> bool:
"""Check if a version string corresponds to a stable release.

Parameters
----------
version : tuple or str
Version string to check.

Returns
-------
bool
``True`` if the version is stable, ``False`` otherwise.

Examples
--------
Stable version examples: ``0.4.12``, ``0.4.1``.
Non-stable version examples: ``0.4.15beta``, ``0.4.15rc1``, ``0.4.15dev0``.
"""
if not isinstance(version, tuple):
version = tuple(version.split("."))

return not LETTERS_PATTERN.search(version[-1])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will be incorrect for a stable "post release". Is it possible to use the parsed version here and check the is_prerelease property?

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, will change!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some test cases for post releases in here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding TODOs

package_name: str,
build: Optional[str] = None,
channels: List[str] = [DEFAULT_CHANNEL],
reverse: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking: make this boolean a kw-only arg to avoid confusion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure I understand 🙃

    package_name: str,
    build: Optional[str] = None,
    channels: List[str] = [DEFAULT_CHANNEL],
    *,
    reverse: bool = False,

This?

"""Conda installer."""

def __init__(
self, use_mamba: bool = True, pinned=None, channels=(DEFAULT_CHANNEL,)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: make use_mamba a keyword-only arg

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as before? 🤔

Comment on lines +40 to +41
self._processes = {} # type: Dict[job_id, subprocess.Popen]
self._bin = None # type: Optional[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

Update to new-style type annotations.

Comment on lines +37 to +38
version = version.rstrip(".*") # ?
version = version.rstrip("*") # ?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure what this is meant to do, but rstrip removes all trailing characters in the given list of characters, so this second call is redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, need to fix this˜

str
The normalized package name.
"""
return re.sub(r"[-_.]+", "-", name).lower()
Copy link
Contributor

Choose a reason for hiding this comment

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

huge nitpick as I'm not sure what characters are even valid in package names, but casefold() may be more appropriate than lower() here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a note

Comment on lines 9 to 14
new_items: Tuple[Any, ...] = ()
for item in items:
if item not in new_items:
new_items += (item,)

return new_items
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new_items: Tuple[Any, ...] = ()
for item in items:
if item not in new_items:
new_items += (item,)
return new_items
return tuple(dict.fromkeys(items))

I'm not sure it's worth having this separate file for just this function (used once from what I see)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, there were more utilities here, but I ended up refactoring them out

def plugin_versions(
url: str,
) -> List[str]:
"""Return information on package plugins from endpoint in json.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we will need to document this API generally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely, will add the docs

Comment on lines +174 to +180
cmd = ""
with open(path) as fh:
for line in fh:
if line.startswith("Exec="):
cmd = line.split("=", 1)[1].strip()
if cmd:
ret_code = os.system(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

😳

I shouldn't be surprised there's no better way to do this. Apparently gtk-launch should do it but obviously not on every system.

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, this is a bit of a "weird part" of the code. Used some suggestions from @jaimergp work on menuinst

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.

3 participants