marcom / Llama.jl

Julia interface to llama.cpp, a C/C++ library for running language models
MIT License
23 stars 2 forks source link

Add formatter spec #8

Closed svilupp closed 7 months ago

svilupp commented 8 months ago

Options:

marcom commented 8 months ago

Lets wait with this after the other PRs have been merged, as it's a lot of code churn

svilupp commented 8 months ago

Sure! But sooner we join it, the better. I’ll rebase once the other two are merged.

marcom commented 8 months ago
svilupp commented 8 months ago

It should be good to go.

Good idea with the "main" protection, but I cannot change it with my permissions.

EDIT: Not ready at all... It seems that we Windows and Ubuntu have different expectations about quotes... Plus it doesn't seem to be picking up the formatted spec, even though it runs in several other CIs I have.

marcom commented 8 months ago

I have added some branch protection rules for the main branch (no delete, no force-push, linear history, all-changes-via-PR). Let me know if you think any of these don't make sense.

I wasn't yet able to configure that checks have to pass, for some reason it doesn't show any checks available. Will investigate, maybe ci.yml needs some changes.

svilupp commented 8 months ago

I forfeit. I removed the formatter step from the CI for now -- this CI is somehow cursed :D (the 1.9-macos keeps randomly failing)

I think we should investigate the CI options as a separate PR:

The most important thing for me is to have the spec defined going forward.

EDIT: After this one, I think we should consider tagging a new release. Any thoughts?

marcom commented 8 months ago

Sorry pressed close by accident.

Lets put this PR on hold for now until we can integrate it with CI.

I think the code formatting issue is not that important for now and doesn't add any new functionality. I also don't really agree that the SciML style, at least as implemented in JuliaFormatter, is always an improvement (e.g. changing all single-line functions to long-form functions, spaces around = in kwargs), but I accept that having some form of consistency is better than nothing, and SciML style is at least well-known in Julia land.

Sorry that I didn't voice these thoughts earlier. We can revisit this later once we can get formatting to be part of the checks run on a PR, otherwise there is a constant drift of code style.

I am trying to fix the Ctrl-C issue and then we can tag a new release.

svilupp commented 8 months ago

Noted!

I hope it's okay that my contributions will come formatted - I have formatter always on in VSCode.

The reason to have it in the package is that then I don't have to keep disabling it and I don't have to keep "un-formatting" the existing code. But our surface area is small atm, so shouldn't be a big deal.

svilupp commented 8 months ago

CI formatter is now fixed. It will run only on 1 slice of the matrix.

Ready to merge if you're okay with it.

marcom commented 7 months ago

Thanks, and sorry that it took so long!