-
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 cli #34
base: main
Are you sure you want to change the base?
Conversation
8932812
to
d502ee3
Compare
4c1f104
to
5e8e11c
Compare
911b8c1
to
4b29b90
Compare
4b29b90
to
c30889d
Compare
308f7cc
to
09032f0
Compare
778fdf9
to
d4e630a
Compare
28459d2
to
8abe33c
Compare
3a9f928
to
b08b4c4
Compare
864717c
to
f303604
Compare
57c947d
to
1e905c4
Compare
@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 🚀 |
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 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.
.pre-commit-config.yaml
Outdated
# - repo: https://github.com/pycqa/isort | ||
# rev: 5.11.5 | ||
# hooks: | ||
# - id: isort | ||
# name: isort (python) |
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 could 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 isort can be replaced by ruff --fix
with the I
rules configured, but I don't think they're part of the defaults.
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.
Removing
- mamba | ||
- packaging | ||
- requests | ||
- pyyaml |
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 menuinst
be listed as a requirement here too?
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 we are relying on a fork so in the meantime it should install that fork. Will add a line
constructor-manager/README.md
Outdated
### Menuinst | ||
|
||
```bash | ||
pip install git+https://github.com/conda/menuinst.git@cep-devel | ||
``` |
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 could be better at the top of the README with the rest of the instructions to install?
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.
Done
constructor-manager/README.md
Outdated
Check for the version of the currently (latest?) installed package. | ||
|
||
```bash | ||
constructor-manager check-version "napari" |
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 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?
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
|
||
__version__ = "0.0.1" | ||
__version__ = "0.1.0" | ||
VERSION_INFO = (0, 1, 0) |
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.
Is this VERSION_INFO
constant needed?
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 really I think 🤔
result = [] | ||
if args.command in _COMMANDS: | ||
result = method(**method_kwargs) | ||
# time.sleep(5) |
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 and the comment below related to time.sleep
should 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, 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" |
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.
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'
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, 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) |
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 should 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
|
||
@lru_cache | ||
def get_config_path() -> Path: | ||
# path = get_prefix_by_name("base") / "var" / "constructor-manager" |
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 should 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.
Fixed
# if not is_spyder_process(int(pid)): | ||
# raise(OSError(errno.ESRCH, 'No such process')) |
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.
Probably this should be removed or updated to detect a napari related process?
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.
Added a note
constructor-manager/README.md
Outdated
constructor-manager check-updates "napari=0.4.16=*pyside*" -c conda-forge | ||
constructor-manager check-updates "napari=*=*pyside*" -c 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.
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.
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.
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.
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'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.
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]) |
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 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?
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, will change!
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 add some test cases for post
releases in here.
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.
Adding TODOs
package_name: str, | ||
build: Optional[str] = None, | ||
channels: List[str] = [DEFAULT_CHANNEL], | ||
reverse: bool = 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.
nitpicking: make this boolean a kw-only arg to avoid confusion
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 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,) |
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.
nitpick: make use_mamba
a keyword-only arg
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.
Same as before? 🤔
self._processes = {} # type: Dict[job_id, subprocess.Popen] | ||
self._bin = None # type: Optional[str] |
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.
Update to new-style type annotations.
version = version.rstrip(".*") # ? | ||
version = version.rstrip("*") # ? |
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'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.
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.
Ok, need to fix this˜
str | ||
The normalized package name. | ||
""" | ||
return re.sub(r"[-_.]+", "-", name).lower() |
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.
huge nitpick as I'm not sure what characters are even valid in package names, but casefold()
may be more appropriate than lower()
here.
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.
Added a note
new_items: Tuple[Any, ...] = () | ||
for item in items: | ||
if item not in new_items: | ||
new_items += (item,) | ||
|
||
return new_items |
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.
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)?
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.
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. |
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 we will need to document this API generally?
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.
Absolutely, will add the docs
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) |
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 shouldn't be surprised there's no better way to do this. Apparently gtk-launch
should do it but obviously not on every system.
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, this is a bit of a "weird part" of the code. Used some suggestions from @jaimergp work on menuinst
Related to #10
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-backend
(this PR)constructor-manager-client
(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-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 withconda
,mamba
andconda-lock
(living in thebase
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 availablecheck-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)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 implementedget-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
Future work