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 a few dotfiles to improve newcomers' experience #1368

Merged
merged 1 commit into from
Feb 5, 2025
Merged

Conversation

yvan-sraka
Copy link
Contributor

WDYT about Astral’s tools, Ruff and uv?

@yvan-sraka yvan-sraka self-assigned this Jan 9, 2025
@yvan-sraka yvan-sraka mentioned this pull request Jan 9, 2025
@rlouf rlouf removed the request for review from brandonwillard January 12, 2025 23:05
.editorconfig Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@rlouf
Copy link
Member

rlouf commented Jan 12, 2025

I don't have anything against uv a priori although last time I tried it installing PyTorch was a headache. We would need to make sure everything works out of the box on every supported platform, and document the decision if we were to go this route.

@yvan-sraka
Copy link
Contributor Author

Yeah, my concern is, iiuc, that for now, we distribute the library without any kind of lock file. I will advocate for at least adding an uv.lock (or similar) to ensure the ability to reproduce a developer environment. Also, if I end up adding support for Nix, I would likely use uv2nix.

@rlouf
Copy link
Member

rlouf commented Jan 13, 2025

Let's open a discussion to weigh pros and cons of uv. ruff is raising some interesting style issues already, I'm not opposed to using it.

@yvan-sraka yvan-sraka marked this pull request as ready for review January 13, 2025 14:10
@rlouf
Copy link
Member

rlouf commented Jan 15, 2025

Overall this looks good to me, and before fixing the code style check I want to understand why ruff asks for explicit re-exports here before merging. An issue on the ruff repository refers to PEP484 but I am still not sure how this is related.

@yvan-sraka
Copy link
Contributor Author

yvan-sraka commented Jan 28, 2025

Overall this looks good to me, and before fixing the code style check I want to understand why ruff asks for explicit re-exports here before merging. An issue on the ruff repository refers to PEP484 but I am still not sure how this is related.

I’ve read a bit about this online, but I’m not sure to have any strong opinion. That said, it seems like ignoring certain lints (as vllm does) while fixing others is a reasonable strategy, WDYT about ignoring E731 and F401 but addressing E721?

Copy link
Contributor

@torymur torymur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and ruff lints to ignore seems reasonable 👍

@yvan-sraka yvan-sraka merged commit 1842fe7 into main Feb 5, 2025
7 checks passed
@yvan-sraka yvan-sraka deleted the dotfiles branch February 5, 2025 11:51
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