Just FYI, within the conda code as it exists right now, imports are kept ordered and clean using PyCharm's Code | Optimize Imports ⌘ ⌥ O with configuration

I would appreciate if we could document these rules somewhere (the wiki might be a good place), as not everyone uses the same tool(s) to edit the code. (I'm using emacs myself, and so would need to manually re-arrange the imports if I knew the rules.)

Yeah I agree. We don't have a good place to do this right now. No time like the present to start.

Are you volunteering ? :-)

Probably should be me, huh 😜

Just curious now, wondering what https://github.com/google/yapf would end up doing to this code base...

The correct place for this is conda/core/repodata.py. That file actually already exists I think. No reason this can't go there.

If you're concerned about combining the files, just rename conda/core/repodata.py to conda/core/repodata_impl.py

To me the new API we are aiming for will be relatively simple (consisting of a few classes / methods only). It's a public API we are designing, not the internal API this uses.
As such, I'd expect this to live directly one level under conda, rather than conda.core.
(I have to admit that I don't fully understand the rationale of the conda package (sub-)structure, but as most of it is not supposed to be public API, I wouldn't spend much time questioning it. But for the public API this is different...)

A big part of the rational is tightly controlling imports. There are READMEs in each of the subfolders, that are duplicated in the docstrings of the subfolders' __init__s.

To illustrate what I meant in my last comment in this thread, I think a user using this API should only import modules directly from conda to get all the public APIs:

from conda.repodata import RepoData
from conda.resolver import Resolver
...

Among other reasons, this makes it clear whenever a user is trespassing into non-public APIs, and it avoids having to maintain the mess that are all the numerous "public" modules including conda.api and conda.exports.

I think there are several very good options we have to indicate what's API and what's not. I might suggest a starting point of, whatever is available with

from conda.api import *

and then take it from there.

Also, a good indicator of what is API should be "does it have a very good docstring"?

Waffling back-and-forth here between staticmethod and classmethod. I think I prefer staticmethod in this case. And we can also propagate that to the PackageCache class for consistency. PackageCache uses all classmethods, but in the case of accessing the private _data/_cache_, I do prefer the staticmethod.

Yeah, I use staticmethod because I don't expect this class to be subclassed, so there is no point for polymorphism and passing a cls argument.

Moreover, even if it is subclassed, in all probability the in-memory caching part will always want to refer to the same data structure instance.

Let's call this _cache_ for consistency with many other classes that use a similar pattern.

Hmm, but this isn't a cache at all. It's the data. (Unless you think of the RepoData object as a proxy that really refers to the in-channel data, so all the local data would be the cache.)
I don't have a strong opinion about the naming otherwise, though would tend to use a single underscore for private members (I don't mind leading or trailing, as long as it's consistent).

I don't like the word "sync" here. It doesn't necessarily, but often does, imply a two-way relationship. And I'm not comfortable with accuracy there.

Yeah, I had the same thought. Would you prefer fetch / fetch_all ?

fetch sort of implies you're explicitly making a remote request. I find myself going back to the word load / load_all. "I don't care how you load it, whether you go out and get it from the internet, or pull it from some other local cache, just populate the data so I can start using it."

Good point.

What are the correct arguments here? These definitely don't feel right to me. Right now I think

def __init__(self, canonical_channel_name, subdir):

This class is obviously going to have strong interaction with the Channel object. The rest of the information in the original __init__ we should be able to get via that interaction.

I admit I merely cloned from the existing code, where the above 3-tuple is used. Yeah, the main ID of the RepoData is the (canonical_channel_name, subdir) pair which forms the url.
But our current code also requires the other two (though ideally the RepoData itself shouldn't know or care about things like priorities.
I suppose the priority can be encoded using an OrderedDict for the RepoData._data, rather than a simple dict. Where is the name used ?

It might be worth me submitting a PR to this PR. Might be the easiest and clearest way to suggest alternatives like this.

Sure, I'm right now looking into a RepoData metaclass that helps with some of that. I think I can get rid of the 'priority' by using an ordereddict. I'm not sure where the 'name' is used later in the code. It may be a convenience for users that doesn't have any use once the RepoData is instantiated.

I haven't changed the RepoData constructor signature (yet) because the object needs to know its priority (lots of lower-level repodata functions require it right now). Similar for the name ('schannel'). We may consider splitting the url argument into 'channel_id' and 'platform' (or 'canonical_channel_name' and 'subdir' if you insist ;-) ), but I consider this a detail at this point.
Again, my goal is to inject the new API in a minimally invasive way, and using the existing __init__ signature seemed natural for that.

Whoa. So you're combining class _data with instance _data? Do they not have different purposes? Or, if you're using a _data attribute at the instance level, how is the clean() method above even relevant?

Combining ? I think they are distinct. The RepoData._data is the dictionary of RepoData objects. (See my earlier rationale for using global / static data).
The instance _data is the actual data, i.e. the "index" right now.

Yes, they are distinct. But having instance object _data shadow the class object _data, and then furthermore having them follow different generic interfaces, is confusion that's easy to avoid.

The RepoData._data isn't supposed to be user-accessible, or in any other way get in conflict with the instance member. But OK, I get your point. I'll rework that (in my metaclass patch).

With my last commit this has become RepoData._instances, and is created (and manipulated) through the RepoDataType metaclass.

I'm not sure sync and restore should be API-level methods. THIS detail I don't think we should be exposing. That's why I prefer simply a load() method, that would make the right decisions for the API user between sync and restore.

That's a good point. I'm not entirely convinced yet that we can hide the distinction from public eyes. But we should try.

I'd probably put an _ in front of persist() right now too, to be consistent with what I just said above.

Done.

Please review for yourself (and as a general code review also) the PackageCache class. Just like this class, it is managing a collection of collections. That is to say, there are multiple package caches, each package cache being a collection of package cache entries (should be renamed to package cache records). The parallel is that the RepoData class manages multiples repodatas (for lack of a better word), and each repodata is a collection of repodata records.

Yes, I will do the review, but this will take time.
I'm fine with the definition "A RepoData object manages repo-data records." :-)
(To be picky myself: "data" already is plural, thus "datas" is an impossible construct. ;-) )

Yeah, that's actually going to end up being a problem. How would you ever do

for repodata in repodata:

for example?

but this will take time.

Yes, it will take time. This API work is going to be literally impossible though without a full, if not deep, understanding of the current code base.

What would you want for repodata in repodata to mean ? Are you asking how to iterate over the RepoData instances, i.e. a nicer way to write for r in RepoData._data ? Yeah, that would be best implemented by virtue of a metaclass, I suppose.

I was just in general commenting on the convention that a collection of like objects ends in 's'. Which is actually why the return value of the current collect_all_repodata() function is named repodatas, and then that return value is iterated over in the current fetch_index() function.

For avoidance of doubt: the goal here is to insert the new API strategically such that

• the overall workflow isn't changed at all, i.e., all lower-level functions are called as before
• the outer API can now be deprecated.
  Thus, the proposal is to gradually replace get_index() and fetch_index() by RepoData.sync_all()...

While the idea here is to have one RepoData object per channel, I also tried to capture the state globally, such that these objects wouldn't have to be passed around through function calls. Instead, other places such as the resolver code would simply access these objects using static RepoData methods...
(The assumption is that almost all conda operations would start by instantiating RepoData objects for all active channels.)

Oh, I didn't comment here. I don't understand the word "enable" here. And why expose enable and get? Why not make get a lazy call to what you have as enable? Or even better, why not just have the RepoData instantiation take care of all of that for you?

I'm not sure I understand. At some place some code needs to explicitly instantiate RepoData objects from channel URLs (likely the ones a user has "enabled" in his ".condarc" file. The enable method does just that.
The get then is nothing but a simple way to access individual RepoData objects once created. It isn't meant to magically instantiate anything.
Note that I imagine that we do want the ability to access RepoData objects separately (in contrast to current conda code that only uses a single "index" dict that has everything merged into it).

The clean operation seems to be necessary for testing, where subsequent tests need to be able to start from a clean slate. I don't fully understand the need, though, as the above clean operation doesn't do anything, i.e. there is no RepoData "destructor" implemented that does any cleanup or similar.

Hopefully, we can soon adjust all conda code to fetch the index using this method. This will then allow us to much better control the data flow, i.e. once we deprecate the above, we can make sure no index access happens "outside" the public API, at which point we have free reign over data representation changes...

Yup. No objections here.