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

refactor: Move to pub(crate) fn in order to detect and remove unused functions #1805

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

Hofer-Julian
Copy link
Contributor

  • 4d550ec moves from pub fn to pub(crate) fn where possible
  • c46aa7d removes unused functions

@Hofer-Julian Hofer-Julian force-pushed the refactor/pub-crate-fn branch from c46aa7d to b4427f9 Compare August 13, 2024 09:43
@Hofer-Julian Hofer-Julian merged commit 34bf823 into prefix-dev:main Aug 13, 2024
27 of 28 checks passed
@Hofer-Julian Hofer-Julian deleted the refactor/pub-crate-fn branch August 13, 2024 12:10
@zbowling
Copy link

zbowling commented Sep 1, 2024

Heh. A couple of these I was using in magic downstream. I add pixi as a crate dep and call into some of these pub functions that are not pub(crate). Think most I can work around. A few I’ll need to be pub again in our downstream patches (like clap execute commands). Hoping to open source that project soon so it’s easy to see how embed pixi

@Hofer-Julian
Copy link
Contributor Author

@zbowling let us know when we can make things easier for you. And yes, open-sourcing magic would help here

@zbowling
Copy link

zbowling commented Sep 1, 2024

Yeah I'm hoping to get through final approvals to open source in the next month. Some of our changes to Pixi are in a public fork, though, right now. I'd like to get those upstreamed where it makes sense. It's some small stuff mostly (in a few cases making a few APIs pub. It's funny because I'm using Pixi as a library, but it's building toward being a binary, so it doesn't have a well-defined API surface like it does a CLI surface. There was no contract there, so I took on that burden knowningly. :-P

Magic also uses clap as a CLI driver, but to make things easy, I just reference clap structs in pixi and call execute like it does, too. That way I can bring about 80% of the high-level packaging CLI commands into magic and then add in all our bespoke stuff around mojo build system integration, our vscode extension hooks (so we can sniff the current project and get access to the right mojo LSP), support our cloud packaging/deploying system (wrapping things up in docker), and local serving commands (just invoking tools when the max package is added to the current project in the max package). We also change the default implicit channels to include our 3rd party conda repo. And to avoid breaking people, we also change the pixi.lock to magic.lock and the venv directory from .pixi to .magic so if you are using magic and pixi side by side and we are lagging a version or two behind pixi, it doesn't break anything for the user.

That's why we end up dipping into some of the project and high-level CLI APIs in Pixi a bit.

Goal is also to make magic optional so you can use pixi, conda, or mamba if you want, but magic just drives specific tools in the MAX package in a more convenient way at a high level when the project includes any MAX deps. It also gives us some telemetry of what our users are using the most, like which other frameworks they are using with MAX and mojo (like pytorch, onnx, tesnorflow, cuda, etc)

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.

3 participants