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 formatter spec #8

Merged
merged 17 commits into from
Jan 17, 2024
Merged

Add formatter spec #8

merged 17 commits into from
Jan 17, 2024

Conversation

svilupp
Copy link
Collaborator

@svilupp svilupp commented Jan 10, 2024

  • Adds a formatter for better collaboration and easier contributions for first-timers ("sciml" format, feel free to propose others)

  • Ran the formatted on the repo (no code changes)

  • Side thing: added models/ folder to .gitignore. Otherwise, it can kill the git when switching between branches. It's the same canonical folder for models as llama.cpp

Options:

  • We could add it to CI as the final check (is it formatted) to ensure quality doesn't deteriorate

@svilupp svilupp requested a review from marcom January 10, 2024 22:00
@marcom
Copy link
Owner

marcom commented Jan 10, 2024

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

@svilupp
Copy link
Collaborator Author

svilupp commented Jan 11, 2024

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

@marcom
Copy link
Owner

marcom commented Jan 11, 2024

  • lib/LibLlama.jl should be ignored as it is an auto-generated file (created by the script gen/generator.jl) for the low-level C bindings
  • a final CI check for proper formatting makes sense
  • I guess we should also protect the main branch to make sure any new code has the same formatting

@svilupp
Copy link
Collaborator Author

svilupp commented Jan 11, 2024

It should be good to go.

  • ignores lib/ folder now
  • added a CI step

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
Copy link
Owner

marcom commented Jan 11, 2024

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
Copy link
Collaborator Author

svilupp commented Jan 11, 2024

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:

  • I made it as a simple logical step "check if it's formatted, if not, fail the pipeline" (ie, it won't show as a Check)
  • There is https://github.com/julia-actions/julia-format which creates formatting PRs
  • maybe there are other options?

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 marcom closed this Jan 11, 2024
@marcom marcom reopened this Jan 11, 2024
@marcom
Copy link
Owner

marcom commented Jan 11, 2024

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
Copy link
Collaborator Author

svilupp commented Jan 11, 2024

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
Copy link
Collaborator Author

svilupp commented Jan 12, 2024

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

Ready to merge if you're okay with it.

lib/LibLlama.jl Outdated Show resolved Hide resolved
@marcom marcom merged commit 70c84db into main Jan 17, 2024
6 checks passed
@marcom
Copy link
Owner

marcom commented Jan 17, 2024

Thanks, and sorry that it took so long!

@svilupp svilupp deleted the add-formatter branch January 17, 2024 06:36
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.

None yet

2 participants