Skip to content

Contributing guidelines #251

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

Merged
merged 15 commits into from
Mar 29, 2022
Merged

Contributing guidelines #251

merged 15 commits into from
Mar 29, 2022

Conversation

st--
Copy link
Member

@st-- st-- commented Jan 27, 2021

So far only contains initial notes on how to make a release (thanks @theogf for the explanation). Just to get the ball rolling. Please feel free to push to this branch and add to it!

@theogf
Copy link
Member

theogf commented Jan 27, 2021

I think the triggering of the registration bot can only be done by people of the organization though

@devmotion
Copy link
Member

Yes, AFAIK @theogf is right, this can only be done by us. So I think it is not really relevant for general or one-time contributors. More generally, like many Julia packages KernelFunctions.jl uses the ColPrac contributor guide and links to it in the README.

@devmotion
Copy link
Member

If we want to add a specific CONTRIBUTING.md file, maybe we could just link to ColPrac? E.g., we could follow the suggestion in ColPrac and add

We follow the [ColPrac guide for collaborative practices](https://colprac.sciml.ai/). New contributors should make sure to read that guide.

But since it does not contain much information, I am not sure it is helpful?

@st--
Copy link
Member Author

st-- commented Jan 27, 2021

KernelFunctions.jl uses the ColPrac contributor guide and links to it in the README

I completely missed that badge amongst all the others at the top of the README. I think even a mostly-empty contributing.md file with appropriate links would be valuable.

@st--
Copy link
Member Author

st-- commented Jan 27, 2021

It also saves you from having to explain steps/workflows/processes to new contributors again and again :) so if you're doing anything else that you think others might benefit from knowing, think of where it can be added so it's visible.

1. Merge the bumped version number into `master` (see above); normally this will be the commit of a squash-merged pull request.
2. Go to the Github URL for this merge commit and write a comment saying `@JuliaRegistrator register`.

The next release will then be processed automatically. KernelFunctions.jl does use TagBot so you can ignore the message about tagging. Note that it may take around 20 minutes until the actual release appears and you can edit the release notes if needed.
Copy link
Member

Choose a reason for hiding this comment

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

I just want to clarify (doesn't have to be added here, I think) that the Github releases and tags are nice but not relevant or mandatory for Julia packages (there exist many packages that work just fine without TagBot or Github releases). As soon as the PR by JuliaRegistrator is merged into the general registry, users can update KernelFunctions (well, in principle at least - sometimes it takes a while until the Julia PkgServers update the registry...).

Copy link
Member Author

@st-- st-- Mar 19, 2021

Choose a reason for hiding this comment

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

If not here, should this be clarified elsewhere? Again, this is about how would the process look like for a new contributor?

@devmotion
Copy link
Member

I am still not sure if the information about version numbers and JuliaRegistrator are helpful and important for new contributors. Maybe it would be sufficient to mention ColPrac (which also includes information about version numbers) and to explain how the code has to be formatted? For instance, I assume it would be good to mention that one should run

julia --project=test/ -e 'using Pkg; Pkg.instantiate(); using JuliaFormatter; format("."; verbose=true)'

before opening a PR.

And maybe these things would be more visible in a pull request template than a separate CONTRIBUTING.md file?

@st--
Copy link
Member Author

st-- commented Jan 27, 2021

Here's one data point: I was missing this information and would have liked to see it. Yes, adding how to format is one of those (which I hadn't thought of because I wasn't new to it, having been the one to introduce it 😉)! And maybe another line for how to run the tests. I'd actually find it really helpful to have a small Makefile so I can just type make format or make test... what do you think of including that in the repo?

Adding it to a pull request template would make it even more visible, but clutter every new PR. Though how about adding a single line to the PR template pointing to CONTRIBUTING.md?

@devmotion
Copy link
Member

And maybe another line for how to run the tests. I'd actually find it really helpful to have a small Makefile so I can just type make format or make test... what do you think of including that in the repo?

I can understand the motivation but I think we should not add a make test (regarding the formatting, on a second thought probably most people just use the respective plug-ins for their editors anyway). There are standard ways to test packages in Julia (e.g. in the REPL, terminal, or VSCode) and in my opinion we should not add another non-standard method. I have never seen anything like this in any other Julia package, and hence I think it would be very surprising and unexpected.

I guess this is also why in general I am a bit sceptical regarding the current draft of CONTRIBUTING.md - SemVer and also JuliaRegistrator, TagBot, CompatHelper etc. are standards in the Julia package ecosystem that are used in almost every other Julia package. Thus this information is really not KernelFunctions-specific and widely known. I.e., I think it's only useful for very, very few people which we could just point to ColPrac, SemVer, and all the other resources equally well if needed 🙂 Thus probably it's also not very useful to add this information to every PR...

@st--
Copy link
Member Author

st-- commented Mar 19, 2021

I guess this is also why in general I am a bit sceptical regarding the current draft of CONTRIBUTING.md - SemVer and also JuliaRegistrator, TagBot, CompatHelper etc. are standards in the Julia package ecosystem that are used in almost every other Julia package. Thus this information is really not KernelFunctions-specific and widely known.

It is widely known amongst people who already know it. How welcoming do you want to be to people who may come from the domain (kernels / GPs / probabilistic modelling) but may be new to Julia? If you would welcome their contributions: what can make it easier for them to get on-boarded ?

I.e., I think it's only useful for very, very few people which we could just point to ColPrac, SemVer, and all the other resources equally well if needed slightly_smiling_face Thus probably it's also not very useful to add this information to every PR...

How else would you prefer to point people to those resources ?

What else would you like to change about this draft before it gets merged?

@st-- st-- marked this pull request as ready for review March 19, 2021 15:01
@theogf
Copy link
Member

theogf commented Mar 19, 2021

I agree with @devmotion that this is not relevant to contributors unless they have writing rights on the repo.
This might even lead to more confusion as they might try to do this themselves and not understand why it's not working.
In terms of contributing guidelines for repo owners there exist ressources already on how to do this :)
EDIT: I am talking about the part on how to register a new package version

@theogf
Copy link
Member

theogf commented Mar 22, 2021

About the contributing guidelines, it would be great to add a few explanation about our testing systems:

  • How do I test the added kernel succeed in the AD tests?
  • How do I test the kernels at all?

@st--
Copy link
Member Author

st-- commented Jun 17, 2021

I'd be keen to pick this up again. I've been struggling personally with figuring out how to actually run the Julia development workflow. It would be great if packages like those of JuliaGaussianProcesses could convince more researchers to join the development of Julia-based code by making it easy for people to discover a package and get started. This could be as simple as some pointers to further reading. For everyone who's been involved in the package development for a longer time this may seem trivial, but it can be a significant - and unnecessary - barrier to entry for people who might be familiar with the maths, even good t coding, but new to the Julia ecosystem. (And just googling for "julia package development workflow" doesnt) So what can we do to make it easier for people to pick up new tools and contribute back to them?

Adding to @theogf's points, here are some more common use-cases that I think would be good to document:

  • How do I run the package tests locally (so I don't embarass myself when proposing a pull request)?
    • This should include instructions for how to format the code, ideally both pointing to editors (NB- VSCode's default Julia formatter seems to disagree with JuliaFormatter/style=blue; if anyone can point out how I can fix that that'd be great) and in the command line (which will always work).
    • Anything else that I should do to ensure I make the reviewers' lives (and therefore my own) as easy as possible?
    • How do I run some (but not all) of the examples (e.g. doc tests are slow as mentioned in Examples in the docs: base PR for cleaning up notebook workflow #303 (comment))
  • How do I build the docs locally? (will be covered by Examples in the docs: base PR for cleaning up notebook workflow #303)
  • How do I run an example notebook?
  • How do I contribute a new example notebook?

@devmotion
Copy link
Member

VSCode's default Julia formatter seems to disagree with JuliaFormatter/style=blue; if anyone can point out how I can fix that that'd be great

There's a separate VSCode plugin for JuliaFormatter: https://github.com/domluna/JuliaFormatter.jl#editor-plugins

@theogf
Copy link
Member

theogf commented Jun 17, 2021

VSCode's default Julia formatter seems to disagree with JuliaFormatter/style=blue; if anyone can point out how I can fix that that'd be great

There's a separate VSCode plugin for JuliaFormatter: https://github.com/domluna/JuliaFormatter.jl#editor-plugins

In my experience it is terrible... I just end up calling JuliaFormatter myself.
It just create processes which are sometimes never killed and it has a huge latency...

@devmotion
Copy link
Member

I think it works pretty well but maybe something changed in recent versions. IIRC it also uses special commandline options that reduce latency.

@willtebbutt
Copy link
Member

@st-- what's the status of this PR? Is there anything that you need help with in order to get it merged?

Co-authored-by: willtebbutt <wct23@cam.ac.uk>
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

I think there are problems with the Makefile. Additionally, do we actually want to recommend its use? make test doesn't seem much shorter than running ] test in a Julia REPL started with julia --project=. (or using the functionality available in VSCode and other editors).

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@st--
Copy link
Member Author

st-- commented Aug 26, 2021

Additionally, do we actually want to recommend [Makefile] use?

As a new user I'd find it really valuable to have an easy way to run the "canonical" version of a command.

@devmotion
Copy link
Member

As a new user I'd find it really valuable to have an easy way to run the "canonical" version of a command.

I would like to improve the workflow, I guess I'm not sure about the Makefile mostly because KernelFunctions would be the only Julia package that I know that would include and suggest to use a Makefile. I don't know any tools in the ecosystem that are available for this workflow and I am worried that it might be more confusing than helpful for contributors and users since it is completely different from what is commonly done in the Julia package ecosystem. Whereas, e.g., there exist already tools for JuliaFormatter in VSCode, Atom, Emacs, and Vim.

st-- and others added 2 commits March 29, 2022 22:35
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@st-- st-- requested a review from willtebbutt March 29, 2022 19:37
@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #251 (1a3f213) into master (3028ecc) will decrease coverage by 22.34%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #251       +/-   ##
===========================================
- Coverage   93.13%   70.78%   -22.35%     
===========================================
  Files          52       52               
  Lines        1252     1246        -6     
===========================================
- Hits         1166      882      -284     
- Misses         86      364      +278     
Impacted Files Coverage Δ
src/basekernels/sm.jl 0.00% <0.00%> (-100.00%) ⬇️
src/basekernels/gabor.jl 0.00% <0.00%> (-100.00%) ⬇️
src/basekernels/cosine.jl 0.00% <0.00%> (-100.00%) ⬇️
src/basekernels/nn.jl 0.00% <0.00%> (-97.62%) ⬇️
src/basekernels/constant.jl 5.55% <0.00%> (-94.45%) ⬇️
src/basekernels/wiener.jl 0.00% <0.00%> (-92.86%) ⬇️
src/basekernels/piecewisepolynomial.jl 0.00% <0.00%> (-89.48%) ⬇️
src/basekernels/rational.jl 20.00% <0.00%> (-80.00%) ⬇️
src/basekernels/exponentiated.jl 0.00% <0.00%> (-80.00%) ⬇️
src/basekernels/matern.jl 25.00% <0.00%> (-75.00%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3028ecc...1a3f213. Read the comment docs.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

I can't see any harm in merging this as-is.

@st-- st-- merged commit c92f2e5 into master Mar 29, 2022
@st-- st-- deleted the st/contributing_docs branch March 29, 2022 19:59
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.

5 participants