Parse and use explicit platform specifications. by afranchuk · Pull Request #1033 · mamba-org/mamba

afranchuk · 2021-06-23T12:32:36Z

Rather than trying to figure out whether a channel string/url contains a platform, we instead make platform specification explicit with CHANNEL[PLATFORM1,PLATFORM2,...].

There is a refactor of the channel class here to hide implementation details/functions which are only relevant for testing.

This is a rework related to discussion at #934, and will close #794.

Note that this introduces breaking CLI changes as well as breaking Python API changes (the Channel class is improved), so these should be noted in the changelog.

afranchuk · 2021-06-23T12:37:09Z

The clang-format linting failed, but it's all on files that I never touched? I automatically clang-format any files I change.

afranchuk · 2021-06-23T14:40:11Z

The other failing tests seem to be using an installed version of mamba rather than one built from source? That's why the python tests are failing (they work if I run pytest myself).

wolfv · 2021-06-24T06:25:53Z

-            create_args.append("--allow-softlinks", no_dry_run=True)
+            create_args.append("--allow-softlinks")
        if always_copy:
-            create_args.append("--always-copy", no_dry_run=True)


I think we should keep the no_dry_run=True here because we check if we're linking the packages correctly by looking at inode etc.

Uh, that fails when I run the test, saying you can't have keyword args in list.append? no_dry_run=True is there just below in the call to create.

afranchuk · 2021-06-29T20:41:29Z

@wolfv Any comment on the failing tests? Do I need to rebase on master again?

wolfv · 2021-06-29T21:17:40Z

@afranchuk I am currently pushing a lot to get this released: openSUSE/libsolv#457

Will have to take a deep dive into this PR after.

I think some issues should be easily fixable, though (e.g. AttributeError: 'Channel' object has no attribute 'platform_urls' ...

I think there are legitimate failures.

afranchuk · 2021-06-30T14:00:50Z

I was able to reproduce the pytest errors (I didn't realize I had accidentally excluded them before), and I think I fixed all of them (by changing how some parts of the python code work).

afranchuk · 2021-06-30T14:04:44Z

I can run clang-format on everything, but it's still a bit odd that the linting for that is failing on files I never changed. Though maybe the explanation is simple: I'm using a newer version of clang-format than the CI, which differs a bit with the older versions. This explains the CI saying files change and my local clang-formatting changing many files as well. I'm not sure what the correct approach for such differences is (is there some way to make it more strictly interpret the .clang-format file?).

afranchuk · 2021-06-30T14:13:48Z

Looks like there's still one test failing, let me see if I can track down why.

wolfv · 2021-06-30T17:29:00Z

The CI mentions:

platform_url(): incompatible function arguments. The following argument types are supported:\\n    1. (self: mamba.mamba_api.Channel, platform: str, with_credentials: bool = True) -> str\\n\\nInvoked with: [linux-64,noarch], 'linux-64'; kwargs: with_credential=False\

afranchuk · 2021-06-30T18:00:13Z

Bah, simple typo.

afranchuk · 2021-06-30T18:01:23Z

Weird though, I ran the tests myself and it didn't have that error.

wolfv · 2021-06-30T18:19:33Z

alright, looks like one remaining issue with windows for mamba: stdout = b'{\r\n "command": "C:\\Miniconda\\Scripts\\mamba create -p C:\\Users\\runneradmin\\AppData\\Local\\T...ict[split_anaconda_token(remove_auth(c))[0]]\nKeyError: 'file://d/a/mamba/mamba/test/channel_b/win-64'\n"\r\n}\r\n'

Maybe it's the trailing slash? or the // vs\\...?

afranchuk · 2021-06-30T18:33:27Z

Yeah, unfortunately at the moment I don't have a windows machine to fix the windows errors more rapidly, but I'll try a few things.

wolfv · 2021-06-30T18:41:10Z

I can debug it on windows on the next days if this doesn't work out :)

afranchuk · 2021-07-01T00:24:25Z

I added some context to the exception, it looks like it's being queried with file://d/... but the actual paths are file://D:/.... I guess I'm missing some sanitization step?

afranchuk · 2021-07-01T18:16:13Z

It's not immediately obvious what is altering the file path to d rather than D:; based on existing tests I think D: is the desired/correct path component.

afranchuk · 2021-07-01T18:26:38Z

I think the issue is the remove_auth conda function being called, which calls parse_urls which has this: https://github.com/conda/conda/blob/33a142c16530fcdada6c377486f1c1a385738a96/conda/_vendor/urllib3/util/url.py#L186. I assume the difference is that now we use api.Channel to parse urls rather than conda's Channel, which results in a mismatch.

afranchuk · 2021-07-01T18:28:43Z

Basically their implementation is wrong for windows file:// uris.

Rather than trying to figure out whether a channel string/url contains a platform, we instead make platform specification explicit with `CHANNEL[PLATFORM1,PLATFORM2,...]`. There is a refactor of the channel class here to hide implementation details/functions which are only relevant for testing.

The python tests will probably still need some more changes.

Some of these fixes have nothing to do with the changes in this branch.

c++) to fix pytest errors.

This removes the call to conda's prioritize_channels, it's not clear whether this is actually necessary.

afranchuk · 2021-07-01T20:08:34Z

Okay, looks like all that's wrong is the linting (though I can't fix that myself apparently since my clang-format disagrees with a lot). If you're okay with the workaround I did for that windows issue it should be in a good state.

wolfv · 2021-07-01T20:17:28Z

Great work! Fixing the linting is easy for me (just run pre-commit run —all)

I’ll need to have a deep dive into this refactor. I saw that you’re using std::optional which is one thing I’m not sure about (since we need to support pretty old macOS versions) but we could quite easily refactor that to pointers or similar

wolfv · 2021-07-02T08:55:27Z

I just ran the linter, let's see if it passes now :)

afranchuk · 2021-07-02T12:44:09Z

Awesome! Yeah I have clang-format 12, beware of differences in the future!

afranchuk · 2021-07-02T12:48:08Z

 //
 // The full license is in the file LICENSE, distributed with this software.

+#include <set>


No longer needed.

Remove

wolfv · 2021-07-05T13:48:08Z

I am testing this PR right now, gonna write down observations as I go:

When using micromamba, and I have channels: [conda-forge] configured in my condarc file, I get this download:

(base) ➜  cbuild git:(explicit-platforms) ✗ micromamba create ninja -n tlinx -c "conda-forge[linux-64, noarch]"

                                           __
          __  ______ ___  ____ _____ ___  / /_  ____ _
         / / / / __ `__ \/ __ `/ __ `__ \/ __ \/ __ `/
        / /_/ / / / / / / /_/ / / / / / / /_/ / /_/ /
       / .___/_/ /_/ /_/\__,_/_/ /_/ /_/_.___/\__,_/
      /_/

Found conda-prefix at '/Users/wolfvollprecht/micromamba/envs/tlinx'. Overwrite?: [y/N] y
conda-forge/osx-arm64    [====================] (00m:00s) Done
conda-forge/noarch       [====================] (00m:02s) Done
conda-forge/noarch       [====================] (00m:02s) Done
conda-forge/linux-64     [====================] (00m:06s) Done

IMO the command line here should overwrite the other configured conda-forge config, shouldn't it?

wolfv · 2021-07-05T13:50:52Z

there is currently no implicit noarch (when explicitly selecting channels).

This means this fails now like this:

(base) ➜  cbuild git:(explicit-platforms) ✗ micromamba create ninja -n tlinx -c "conda-forge33[linux-64, osx-arm64]" --override-channels

                                           __
          __  ______ ___  ____ _____ ___  / /_  ____ _
         / / / / __ `__ \/ __ `/ __ `__ \/ __ \/ __ `/
        / /_/ / / / / / / /_/ / / / / / / /_/ / /_/ /
       / .___/_/ /_/ /_/\__,_/_/ /_/ /_/_.___/\__,_/
      /_/

conda-forge33/osx-arm64  [====================] (00m:00s) 404 Failed
conda-forge33/linux-64   [====================] (00m:00s) 404 Failed
Encountered problems while solving:
  - nothing provides requested ninja

ERROR   Could not solve for environment specs

I think it would be better in this case to fail with a donwload error of some sort (which we did otherwise with the existence of the noarch channel). If we explicitly request some platforms, we coudl require that all of them resolve to something on the server..?

wolfv · 2021-07-06T06:38:25Z

Let's fix the remaining issues on master :)

afranchuk · 2021-07-06T14:27:15Z

Sounds good! As to your comments:

IMO the command line here should overwrite the other configured conda-forge config, shouldn't it?

I'm not so sure. You could certainly make a decision here about how you think it should behave, but I think the behavior you saw is okay/expected, same as if you had specified -c conda-forge[linux-64,noarch] -c conda-forge. I understand why you might want to have the behavior that you expected, but that's kind of what --override-channels is for. You could certainly make that other case work the way you expect, but I think that's more complicated and not quite as straightforward to understand.

I think it would be better in this case to fail with a donwload error of some sort (which we did otherwise with the existence of the noarch channel). If we explicitly request some platforms, we coudl require that all of them resolve to something on the server..?

This seems like an error priority issue. I'd argue that any 404 received for a configured channel should take precedence as a final user-visible error over an error that a package could not be resolved. Are there any use cases where you'd want a 404 to be ignored?

wolfv · 2021-07-06T14:29:13Z

@afranchuk yes, if you request a channel that doesn't implement the default platform the error should be ignored.

e.g. you can use defaults from osx-arm64 but you'll only get the noarch platform. When mixing channels + platforms it's likely that it might sometimes fail

wolfv · 2021-07-07T18:15:27Z

@afranchuk it seems that the load_channels implementation also changed slightly, correct? Would you mind detailing what I'll have to adjust in boa to make it work?

https://github.com/mamba-org/boa/blob/5482bd4a8eea22c93d7f2d87692e93f0264f9876/boa/core/solver.py#L110-L112

Currently it doesn't seem to load the noarch platform.

Maybe we could actually revert this behavior in a way that keeps boa working?

afranchuk · 2021-07-08T19:21:28Z

@wolfv load_channels should be functionally identical, however the index it returns is different. The index has the same first tuple items (Subdir), however the second tuple items (which used to be conda.Channel) are now a dict with the following entries:

channel: the mamba.Channel
platform: the platform name (string)
url: the channel url (string)

There should probably not be a tuple, opting for a subdir entry in the dict instead. But I didn't make that change because I think in a few places the dict (previously channel) is used on its own.

wolfv · 2021-07-08T20:12:30Z

I think one problem is that when giving a platform explicitly now noarch isn't implicitly loaded. That's breaking boa right now but I'll come up with a fix tomorrow :)

afranchuk · 2021-07-08T20:27:06Z

Oh yeah you're right, load_channels is the same but the use of mamba.Channel rather than conda.Channel changes that resolution behavior.

afranchuk mentioned this pull request Jun 23, 2021

Updates to support arbitrary channel platforms. #934

Closed

afranchuk commented Jun 23, 2021

View reviewed changes

Comment thread src/mamba/py_interface.cpp Outdated

wolfv reviewed Jun 24, 2021

View reviewed changes

afranchuk force-pushed the explicit-platforms branch from bd1430a to 14914ac Compare June 30, 2021 13:59

afranchuk added 9 commits July 1, 2021 14:55

Fix a few of the tests.

706e8fd

The python tests will probably still need some more changes.

A few more test fixes.

d41a96f

Some of these fixes have nothing to do with the changes in this branch.

Rework the python implementation (and add supporting functionality to

25a7d61

c++) to fix pytest errors.

Fix mamba env to not resolve channels using conda.

06edf15

This removes the call to conda's prioritize_channels, it's not clear whether this is actually necessary.

Fix typo in utils code.

f2c9940

Remove dead code.

d85dd09

Elaborate on error when channel key is missing in utils.py.

cd4dbae

Do not adjust file:// urls when looking up channels to link.

feec101

afranchuk force-pushed the explicit-platforms branch from 897c2e3 to feec101 Compare July 1, 2021 18:55

Correctly resolve channels in API install function.

bd72a27

run linter

6fe0176

use clang 11.1 for pre-commit

0ea5f27

afranchuk commented Jul 2, 2021

View reviewed changes

wolfv merged commit c5f9a15 into mamba-org:master Jul 6, 2021

adriendelsalle mentioned this pull request Aug 3, 2021

Fix mamba not writable cache error #1108

Merged

Conversation

afranchuk commented Jun 23, 2021

Uh oh!

afranchuk commented Jun 23, 2021

Uh oh!

Uh oh!

afranchuk commented Jun 23, 2021

Uh oh!

wolfv Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

afranchuk Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

afranchuk commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfv commented Jun 29, 2021

Uh oh!

afranchuk commented Jun 30, 2021

Uh oh!

afranchuk commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afranchuk commented Jun 30, 2021

Uh oh!

wolfv commented Jun 30, 2021

Uh oh!

afranchuk commented Jun 30, 2021

Uh oh!

afranchuk commented Jun 30, 2021

Uh oh!

wolfv commented Jun 30, 2021

Uh oh!

afranchuk commented Jun 30, 2021

Uh oh!

wolfv commented Jun 30, 2021

Uh oh!

afranchuk commented Jul 1, 2021

Uh oh!

afranchuk commented Jul 1, 2021

Uh oh!

afranchuk commented Jul 1, 2021

Uh oh!

afranchuk commented Jul 1, 2021

Uh oh!

afranchuk commented Jul 1, 2021

Uh oh!

wolfv commented Jul 1, 2021

Uh oh!

wolfv commented Jul 2, 2021

Uh oh!

afranchuk commented Jul 2, 2021

Uh oh!

afranchuk Jul 2, 2021 • edited by wolfv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfv commented Jul 5, 2021

Uh oh!

wolfv commented Jul 5, 2021

Uh oh!

wolfv commented Jul 6, 2021

Uh oh!

afranchuk commented Jul 6, 2021

Uh oh!

wolfv commented Jul 6, 2021

Uh oh!

wolfv commented Jul 7, 2021

Uh oh!

afranchuk commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfv commented Jul 8, 2021

Uh oh!

afranchuk commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

afranchuk commented Jun 29, 2021 •

edited

Loading

afranchuk commented Jun 30, 2021 •

edited

Loading

afranchuk Jul 2, 2021 •

edited by wolfv

Loading

afranchuk commented Jul 8, 2021 •

edited

Loading