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 Repo.set_channel() #2352

Closed
wants to merge 3 commits into from
Closed

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Mar 6, 2023

I am trying to use libmambapy with already downloaded repodatas from conda side. I just want libmamba to load a given JSON for a given channel.

I've tried setting context.offline = True and other tricks so SubdirData finds the downloaded JSON just fine, but there's some disagreement in how caching works right now, and actually at that point we already know that there's no download needed, and I have the direct path anyway.

As a result, I am trying to load it use libmamba.Repo directly, but the Python-exposed constructors differ from the ones SubdirData is using.

I am assuming this is the reason why I can't use api.Repo directly with channel containing MatchSpec like conda-forge::python: libsolv will say "unsupported request" for these!

So if I can set a channel separately, then things should work. This PR adds a couple bindings in libmambapy, hoping that's it :D

@jaimergp
Copy link
Contributor Author

jaimergp commented Mar 8, 2023

This seems to fix the issue I was seeing in conda/conda-libmamba-solver#65!

@jaimergp jaimergp marked this pull request as ready for review March 8, 2023 08:25
@jaimergp jaimergp closed this Mar 14, 2023
@jaimergp jaimergp reopened this Mar 14, 2023
@wolfv
Copy link
Member

wolfv commented Mar 14, 2023

Looks good to me!

@JohanMabille
Copy link
Member

JohanMabille commented Mar 14, 2023

Hey, sorry for the late review. Would you consider binding the constructor used by SubdirData instead of adding a setter for the channel? I think it makes more sense to fully initialize an object within its constructor rather than partially initiliaze it in the constructor and later call a setter to finish the job. Also, there is a chance that we change the API of the constructor accepting strings only (or even that we remove it), since it does not fully initialize the repo object.

@jaimergp
Copy link
Contributor Author

jaimergp commented Mar 16, 2023

Makes sense! I can give it a try; thanks for the review @JohanMabille.

That said, that means we also need to expose RepoMetadata. And in a way, the str-based constructor was incomplete to begin with (the channel is missing). Should we maybe set the channel internally in the str-constructor by creating a Channel out of the url right away? Something like:

    MRepo::MRepo(MPool& pool, const std::string& name, const std::string& index, const std::string& url)
        : m_url(url)
    {
        m_repo = repo_create(pool, name.c_str());
        m_repo->appdata = this;
        read_file(index);
        // not true C++, just me making it up
        const Channel channel = Channel(url);
        p_channel = &channel;
    }

@AntoinePrv
Copy link
Member

@jaimergp I'm working on changes that will make possible to use a Pool and Solver directly from a repodata.json without interacting with the rest of Mamba (downloads etc). Would that be of interest?

@jaimergp
Copy link
Contributor Author

Hi! Yes, that's exactly the use case we are after. We are directly instantiating Repo and Solver, but in that API the channel info is missing. Hence this PR.

@AntoinePrv
Copy link
Member

What is you timeline? We're about to to a Mamba release so we could try to get this one in. Otherwise the larger refactoring will be in another release where we will also break libmamba API (maybe a couple months).

@jaimergp
Copy link
Contributor Author

Ideally we can get this one in now 🙏

@wolfv
Copy link
Member

wolfv commented Mar 22, 2023

maybe we can make a quikc follow up release? cc @JohanMabille @AntoinePrv

@JohanMabille
Copy link
Member

Closing this one in favor of #2398 which is definitely simpler and safer.

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.

None yet

4 participants