-
Notifications
You must be signed in to change notification settings - Fork 1
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
Software sustainability #41
Comments
Wow, this is the first time I read these CII criteria, very exhaustive, nice! Under "Warning flags" it basically says we should use a linter in strict mode. I think this can indeed be very useful. I once did a project with pylint at maximum strict mode. At first it's very annoying, but in the end I was very happy with the result, even though I sometimes disagree with the defaults. Strict mode also notifies you that you should write doc-strings for all functions and modules. This way we would automatically satisfy the documentation criteria. Of course, we don't want to get into holy formatting wars ;) But I think we are actually already very close in the way we write code. I would say that we just configure pylint to ignore the things we still turn out to disagree on, which is easy enough to do. Then we can also add a linting check to CI. I would also then activate the linter in my editor, so that it becomes easy to fix some warnings along the way (i.e. while you may be doing something else). |
Sounds good to me. It will also force me to finally set the linewidth properly. I still get warnings when the line exceeds 80 characters, while I think we once talked about setting that to 120. |
This comes from the software sustainability plan that the eScience center asked us to fill. 😃 I didn't know about that either. |
Ahaha, right, clearly I wasn't involved in that ;) |
Yeah, I personally tend to set it to 120, but I have no issues making it bigger still, or ignoring the rule entirely. There are a lot of rules that are far more useful than this :) |
Other point we should discuss I think is the versioning of the code. I would like to add a DOI to the repository through Zenodo, which can automatically archive the different releases of the code. Making such releases on a consistent basis would be good (we have one so far). |
With commit 54fc340, I fixed the LabelEncoder warning (code quality improvement, CII section "Warning flags"). |
Another warning fix should hopefully come from wandb/wandb#2129 |
Wandb PR has been merged, so that's another warning gone after updating to wandb 0.10.30. |
All Python warnings from running the experiments are gone now, also #90. A next logical step would be to "fix" all current linter warnings produced by flake8. After that, we could see what pylint on extreme strictness levels suggests, maybe it has some useful ideas. |
Ok, made a PR for a first run of flake8 fixes: #91. For next steps, we should probably make a list of things we need to do to fulfill the CII criteria. Let's start out just copying it and striking through items we have already. I'll do this in a new issue to keep this one from becoming too long and messy. In fact, I'll just go ahead and split up everything I think is still unresolved from this thread into separate issues, so we can close this and continue there:
|
Define actions still required by the software sustainability plan and implement them.
Filling the Cll Best Practices Badge Program checklist + post the results (badge in the README)--> Cll Best Practices Badge Program "passing" checklist #93Any part worth contributing back to pytorch?--> Contributing back into PyTorch #92The text was updated successfully, but these errors were encountered: