Fix inaccuracies in (and possibly remove) "skills"

[DePasqualeOrg]

While I was working on #118, I noticed many inaccuracies in the "skills" documents that were added by @ronaldmannak in #92. I corrected some of these in #118 and have corrected more here. However, I think it would be best to remove most of these documents for the following reasons:

• They appear to be mostly AI-generated summaries of source files. It's better for agents to directly read the source files, which should be self-documenting.
• They are liable to get out of sync with the codebase, as happened with the latest refactoring, and are thus detrimental to agents that use them.

A better use of skills is to record knowledge that isn't directly apparent from the codebase. Only the porting guide fits this criterion. I suggest that the other documents be removed.

[davidkoski]

From my experience, skills are useful at reducing token use -- the skills are much smaller than the codebase. Yes, refactoring might leave them vulnerable, but they can be updated. It is a risk with any documentation that is not the code itself. Even the code can have dangling bits that are not used or should not be used.

Before we cut the tag for 3.x I would like to take a look at some of the functions in Evaluate: it has evolved over time and I think we have too many entry points that require maintenance.

Anyway, an update to the skills is a good idea.

[davidkoski]

I think removing this makes sense -- it is private to the file and part of the implementation of other pieces.

[davidkoski]

but I think this one is useful for documenting how some of the implementation works.

[davidkoski]

ah, but you added in its own section below. that seems fine

[davidkoski]

This might be useful, but let me write why:

Functions that take parameters that contain MLXArray, such as LMInput or Chat.Message should have their parameters marked as consuming. This prevents callers from using these values after passing them to the function -- effectively it transfers ownership to the function.

For example KVCache contains MLXArray and is updated with the LLM context as it runs. Consider a ChatSession that is initialized with KVCache values to restore a session:

struct ChatSession {
    public init(
        cache: consuming [KVCache]
    )
}

The KVCache is transferred to the ChatSession -- this prevents a user from making multiple ChatSession using the same underlying KVCache. It wouldn't be thread-safe and further wouldn't be semantically correct as the ChatSessions would leak to each other. The correct way is to make an independent copy of the KVCache and pass it to each ChatSession.

Something like that.

[davidkoski]

After reading through this more carefully, a lot of it isn't even refactoring, it is just forward progress. I wonder if the skill could contain a pointer to the appropriate area of code -- it would be more like an index. I think the explanatory text (that doesn't live in the documentation itself) would be very good in the skills.

Potentially we could make tests that 1) compile and 2) demonstrate some of the features and reference that. It would make sure it still works and still get the benefit of the skills. I am not 100% sure that works though!

[ronaldmannak]

It looks like I was summoned. If you can "improve" the skills, please do, but do not remove the API reference, since letting the model figure out the API based on the source code is probably not a good idea

[DePasqualeOrg]

These long AI-generated summaries impose a tax on anyone who uses, contributes to, or maintains this repo. They force everyone to pollute their context with information that is mostly duplicated (not additive) and that will inevitably drift from the source of truth.

There are ~4000 lines of "skills" files, compared to ~14,000 lines of library files (excluding tests and model implementations). In practice, models are probably ingesting more context than they would if they just read the source files, which already makes these "skills" a net negative, even before accounting for the negative impact of content drift and maintenance burden.

In my experience, models do fine just reading the source code. That is exactly what they're trained to do. Well-named file paths and well-structured files (for example, breaking long files into extensions) allow for progressive disclosure of complexity, which makes a separate index unnecessary.

I think this repository shouldn't impose a certain agentic approach on users. I will be excluding these files from my local checkout like this so that I never have to deal with them:

git sparse-checkout init --no-cone
git sparse-checkout set '/*' '!/skills'

Please go ahead and make whatever modifications you want to this PR, or close it.

[DePasqualeOrg]

I see that "skills" have also been added to MLX Swift, so I think those should also be reviewed. I have excluded them from my local checkout as well. https://github.com/ml-explore/mlx-swift/tree/main/skills

I think these "skills" cross a line between core library functionality and personal dev environment preferences. They affect everyone's dev environment without them necessarily realizing it, and force those who don't want to accept the downsides to opt out.

I suggest that these "skills" be hosted separately by the author so that people can opt in to this particular workflow and dev environment rather than having to opt out of it.