noaa-fims / collaborative_workflow Goto Github PK

Describe the bug
Describe in the style guide a consistent naming convention

To Reproduce

1. Try to write a templated function
2. Be confused about what to name things

Expected behavior
Be not confused about what to name things

Is your feature request related to a problem? Please describe.
Currently, there is no check that bookdown is able to knit upon changes if working in a branch

Describe the solution you'd like
Add a github action that knits bookdown and stores the html files (or maybe pdf?) as an artifact.

Describe alternatives you've considered
Alternatively, we could just tell folks to render locally, but I think having an automated check is nice. Plus, having a rendered version for reviewers of the pull request can be helpful

Additional context

I can work on this, unless someone else would like to! It would probably good to make this a reusable workflow in the nmfs-fish-tools/ghactions4r instead of making a custom action in this repository.

To start the discussion of how to make a collaborative workflow and what we want the workflow to look like, I will have one-on-one meetings with interested individuals where the discussion will be documented here. Feel free to edit this issue and add your name to the list if you are interested in helping establish a collaborative workflow for FIMS. For those listed below, please check your inbox for a google meet invitation. If you have questions on what we will be working on, please see the README or respond to this issue with your question.

• @cmlegault
• @HTownsendis

Thanks to #52 (implemented in #66), contributors to the workflow doc will be able to view the HTML book as an artifact of github actions rather than rendering the bookdown files locally. If that seems to be working well, we should update or replace the majority of Chapter 1, which is about rendering and previewing the book locally: https://noaa-fims.github.io/collaborative_workflow/index.html#render-book.

I didn't use the "Handbook addition" issue template because I'm not suggesting a [NEW SECTION] as would appear on the subject. Should we could we make a "Handbook edit" template for suggested changes to existing sections?

Describe the bug
There has been some back and forth about whether styling and documentation should be done before pushing commits up or rely on the github actions to do it for you. I think in the interest of minimizing teh number of pull requests hanging open due to documentation, we agreed to ask the developer to do it and add to both the pull request template and the workflow doc here: NOAA-FIMS/FIMS#55

These steps should be added to the doc.

I will try to add this but tagging Bai and Kathryn to be sure I captured the ideas correctly.

Code Bender has a great training program on how to manage a Research Software Project.

• https://merely-useful.tech/py-rse/
• Governance
• Communication
• Workflow
• Newcomers
• Design
• Mechanics
• Leadership
• Change
• Software Engineering
• Robustness
• Partnering
• Findability

During 16 Feb 2022 meeting, @Cole-Monnahan-NOAA noted a difference in assumptions about additive constants added to composition data in multinomial likelihood to make it robust to the presence of zero observations in some bins (constant could be added to both expected and observed or just observed). @jimianelli-NOAA, suggested that neither approach is best.

In the hopes that a github issue like this is a reasonable place to come up with more detail on the specification than what's currently documented on this line:

@jimianelli-NOAA please let us know what you think is the better approach.
@Bai-Li-NOAA, I'm assuming this issue isn't present in the model comparison OM since the likelihoods are all on the EM side of that project, right?

Regardless of the approach chosen as best practice, would it be useful to include the option of alternative approaches to facilitate comparison with existing models?

Can we put Dirichlet-multinomial likelihood on the wishlist for Milestone 2?
I see we have "Weighting and auto-weighting" on our requirements list: https://docs.google.com/spreadsheets/d/1_QRhYpzhRzzE4mon-934b8En0Xxz3JJfNZsd7UTZnxQ/edit#gid=0&range=C13 but I'm not sure how we plan to keep track of potential specifications that are not in Milestone 1.

https://noaa-fims.github.io/collaborative_workflow/code-of-conduct.html#questions-or-reporting-unacceptable-behavior

Also include a way to report concerns for the FIMS leaders (this could be an alternative email that the FIMS leaders don't have access to).

Describe the bug
The links at the top of the bookdown to download a PDF or interact with social media don't work

Expected behavior
Either make them work or remove them

I recommend we adopt a linter - any suggestions @cmlegault @ChristineStawitz-NOAA @k-doering-NOAA @msupernaw?

Problem

No established Code of Conduct for FIMS.

Potential solution

Reference an established Code of Conduct for the FIMS project.

There are many Codes of Conduct out there and, unless necessary, guidance suggests not writing bespoke codes. Ethics.org lays out why organizations should have a code of conduct, Open Source Guides provides links to some examples of Codes of Conduct that are widely used, and GitHub provides a means to add a Code of Conduct to your repository. Some options include

• Contributor Covenant 2.1
• GitHub Event Code of Conduct
• Django
• Citizen
• Openscapes

Feel free to edit this comment to include more links.

Tasks

• Talk Document about Codes of Conduct at a FIMS meeting to bring everyone up to speed
• Decide on Code of Conduct at a FIMS meeting (FIMS guidance says just pick one)
• Provide details on how to report violations of the Code of Conduct
• Establish instructions on how to access the Code of Conduct for when new projects start within FIMS

Title of section
Software specification

Suggested section content and location
This will describe the modules in FIMS milestone 1 and how each of the components are implemented. We will start by focusing on the C++ implementation and then bring in more detail about the R functionality

Additional context
Will move content from https://docs.google.com/document/d/1iSEhJqcpSD269QdABeDE4aBZGqGcBrIrLnS7eMkSYv0/edit#heading=h.70perzbxm1pd to define closed design issues.

Describe the bug
@Andrea-Havron-NOAA pointed out it is hard to navigate the Rmd files and separating out high level and more cookbook sections.

Expected behavior
Separate overview and how to sections.

For VSCode's recommended C/C++ extension, blog posts for Windows users recommend having the MinGW64 installed and on your C drive (and proper path C:\MinGW64\bin). In the development environment section of the FIMS handbook, it says Rtools is sufficient and I know I have gcc on my machine through admb and the mysys2 version of mingw64 through rtools40.

I'm just confused about what exactly I need and if I can use the compilers already on my machine to use VSCode's C/C++ extension to compile programs and debug.

TIA

Title of section
testing case documentation (addresses @k-doering-NOAA 's question from pull request #64

Suggested section content and location
add where testing case documentation should go to chapter 10 testing

Additional context
@ChristineStawitz-NOAA , I noticed that we have M1 model specification in the developer handbook. Do you think we will keep adding M2, M3 specifications to this book? If so, I think we could add testing case documentation to this developer handbook as well. For example, when developers could add testing case documentation table to M1 testing case documentation. What do you think?

@ChristineStawitz-NOAA Would it be useful to add a chapter to the contributors guide outlining roles? Some of this info is in the FIMS TOR, so we could reference that but also elaborate (e.g. use the NMT Testing Engineer as consultant on building your tests).

Title of section
Naming conventions and file organization

Suggested section content and location
This might be documented within exiting sections instead - basically, giving instructions on how to organize code (e.g., should multiple R functions be placed in the same .R file? How do you decide whether to add to an existing file vs. create a new one?)

Additional context

A common convention with testthat files is to give them the same name as R function files with the prefix test- . For example, if myutils.R contains the R code, then the testthat functions that test the code would be in a file called test-myutils.R (in the tests/testthat subdir). I think this could be helpful for FIMS given so many will be touching the codebase, and may need to troubleshoot tests they did not write.

I'm also wondering if we could do something similar with the googletest tests and benchmarks, to make it easier to match up tests with the files that produced them?

Open to other organizational ideas, though! I think whatever is decided on, it would be great to document it in the handbook.

Matt can you please work on Chapter 5.

Branch protection allows for searching branch names with grep functionality to apply merging rules, i.e., protection. This will be helpful to protect the main/trunk branch such that pull requests cannot be merged in prior to passing various checks or by individuals without the authority to do so.

The TTS Handbook (available at 18F/handbook) is a good example of a government agency trying to document guidance on appropriate ways to collaborate and use tools.

I set up a first version of a bookdown workflow: https://github.com/NOAA-FIMS/collaborative_workflow/blob/main/.github/workflows/update-bookdown.yml

I had some questions about how it should be set up.

1. I think a cleaner solution might be deploy the output to a separate branch (perhaps called gh-pages by convention). This eliminates the need to commit output directly to the main branch. I started doing this for the SSMSE manual and have liked it better. Here is what the SSMSE gh-pages branch looks like: https://github.com/nmfs-fish-tools/SSMSE/tree/gh-pages .Basically, it is just the rendered content. Let me know if we should set it up this way!

2. If we like the way the rendering is currently set up (committing the rendered bookdown to the docs folder directly), that is fine - or now, I am setting it up to open a pull request, but we could set it up to automatically commit. I thought it would be good to test it out first with the pull request

Is your feature request related to a problem? Please describe.
It's unclear where to put Rcpp modules vs math functions vs C++ classes when adding code

Describe the solution you'd like
A roadmap for developers to make it clear which folders and header files to put their code in

Title of section
C++ unit testing and benchmarking (existing section)

Suggested section content and location
Add more instructions to the C++ unit testing and benchmarking section in order to answer the following questions (Thanks @k-doering-NOAA for bringing up those great questions!).

• How do we "clean up" after running the tests, since so many files are created? Do we need to delete these, or should they be retained?
• How do we run the tests again (say, after making an edit to a test or c++ code)? What cleanup needs to be done and which commands are used?
• It might be useful to add to the unit test templates what needs to be added to the cmake file each time.
• How do failing tests look? What information do you get? Perhaps the C++ unit testing and benchmarking section should also include an example of a failing test.

Additional context
None.

Is your feature request related to a problem? Please describe.
A style guide is needed for the markdown files that make up the collaborative_workflow book.
Mainly, it is unclear if we should be wrapping lines or not.
Unwrapped lines make the text difficult to read on GitHub,
though this is only a small concern to me.
Guidance would be needed on where and when to wrap lines if we wanted to go that route.
Largely, I just want to know how to best make contributions in a consistent, logical, and appreciated way.

Describe the solution you'd like
An automatic linter could be added as a GitHub Action.
The gh would formats the markdown files before they were merged into main.

Describe alternatives you've considered
I thought about adding information to the following file

I am not sure if there is an action available for .rmd files but there certainly is for .md files.

Additional context
NA

Title of section

Setting up VS code

Suggested section content and location

As @iantaylor-NOAA suggested, this could be an appendix, since it is rather detailed and only applicable to dev team members that are using VS code as their IDE

I thought it would be good to open an issue on this as we all learn more about vs code. I think we should include recommended modules that we found useful, since VS code is flexible and not intuitive to set up. Things worth documenting so far (feel free to edit this comment and add to this list)

• Extensions needed for working with c++ code
• Extensions needed for working with R (good list on the R extension for vs code, recommending other extensions)
  • [edit] New documentation of R setup added to vscode site in March 2022: https://code.visualstudio.com/docs/languages/r
  • R debugger for VS code: https://github.com/ManuelHentschel/VSCode-R-Debugger
  • Note which options need to be added to JSON settings, e.g., "r.plot.useHttpgd": true to use httpgd for plots
  • point out suggested keyboard shortcuts for R: https://github.com/REditorSupport/vscode-R/wiki/Keyboard-shortcuts [Ian used just the first 3]
  • consider listing other tweaks, like the r.alwaysUseActiveTerminal setting discussed here: https://github.com/REditorSupport/vscode-R/wiki/Interacting-with-R-terminals#sending-code-to-r-terminals
• Setting up Liveshare
• Setting up Rewrap (tool for wrapping/unwrapping text lines)
• Github copilot (if we end up liking it)

Additional context

@iantaylor-NOAA put together some instructions for setting up Rewrap

Note if need to set up a new computer, you can save the keybindings.json and settings.json files, then copy them over to vscode. This will save some setup time!

Is your feature request related to a problem? Please describe.
Claudio-Castillo would like join the FIMS conversation with regards to how Multifan-CL.

Describe the solution you'd like
Have a conversation with the developers / users of Multifan-CL

Describe alternatives you've considered

Additional context
Tagging data and movement are the two biggest priorities of Multifan-CL that are only marginally available in SS3.

Thoughts on process for assigning code reviewer? e.g. auto, round robin, load balance

I like load balance (chooses reviewers based on each member's total number of recent review requests and considers the number of outstanding reviews for each member).

More here: https://docs.github.com/en/organizations/organizing-members-into-teams/managing-code-review-settings-for-your-team

The paragraph on user software install contains a lot:

Here are a few suggested revisions based on a planning session for the R Packages training that @k-doering-NOAA and I are working on:

• reformat as a bulleted list and only include OS-specific info under the relevant systems
• require R version 4.0.0 via a change to the https://github.com/NOAA-FIMS/FIMS/blob/main/DESCRIPTION and simply state that as a requirement in this guide
• clarify that that RStudio isn't a requirement (while still noting that version 1.2.5042 or newer is needed if you choose to use it)
• note that the remotes package is needed, or recommend installing devtools which comes with remotes
• Rtools may be the only thing that needs to go under Windows users, and there clarify that IT support is needed to install it on NOAA computers (or any without administrative accounts)

I'm happy to put together a Pull Request, but wanted to offer other the opportunity for input instead of just assuming the choices are the best ones

Just a thought that automating documentation updates could be helpful so that it is always in date. https://noaa-fims.github.io/collaborative_workflow/documentation-template.html#step-by-step-documentation-update-process

ghactions4r includes reusable workflows to update documentation and (if desired) code style for R code. I imagine automating the c++ code is also possible.

Overarching tasks listed in the implementation plan outline

• Summarize available best practice guides for general workflows, commit messages, issue templates and forms, coding practices, etc and place generic information in FIT such that all fisheries tools can benefit from this information.
• Decide on a practice when multiple options exist, such as GitHub flow versus centralized GitHub, for FIMS. These decisions will largely be made by vote from the whole group unless a clear choice has major benefits for FIMS over other options.
• Document choices on GitHub here https://github.com/NOAA-FIMS/collaborative_workflow

https://noaa-fims.github.io/collaborative_workflow/contributor-guidelines.html#fims-branching-strategy

These are actually 2 different things. We want to use trunk-based development.

Description:
https://reviewpad.com/blog/github-flow-trunk-based-development-and-code-reviews/

Problem

What documentation will be required of new features because stand-a-lone documentation that doesn't go into detail about how a feature fits into the bigger picture might not be enough. See #1 for @cmlegault comment that started this thought.

Proposed solution

Require new features be added to an existing vignette before pull request is merged.

Potential problems

• Hard to remember that it is a requirement because it is not standard practice.
• Vignettes might not exist yet.

A few changes to this section that we should make to align this section with the ongoing training from @kellijohnson-NOAA and @k-doering-NOAA: https://noaa-fims.github.io/collaborative_workflow/contributor-guidelines.html#code-review

• revise the Review Checklist section to focus on new github action for PR checklists
• provide more detail on automated tests
  • clarify that developer should write tests prior to the PR
  • make clear that they will be run automatically for each PR
  • note that the results of the tests should be shown on the GitHub PR page

Please edit this issue to add more checklist items as appropriate.

Title of section
software specs

Suggested section content and location
Wishlist for software setup doc: (1) recommended VScode extensions, (2) steps to setup C++ compiling in VScode, (3) steps to setup R terminal (w or w/o radian)
I can google all those things, but would be better for us to build a shared doc
https://www.r-bloggers.com/2021/01/setup-visual-studio-code-to-run-r-on-vscode-2021/

Title of section
Software License

Suggested section content and location
This should make clear which license FIMS is under (any maybe why?) It also should include any hoops contributors might have to go through to get their code into the codebase (e.g., for the shiny R package, you have to sign some sort of release before your PR is merged in if you aren't a primary developer).

Additional context
I saw in the FIMS code repo that GPL3 is the current license choice. I'm curious if this has been discussed and clearly chosen? GPL3 is more restrictive than MIT (although they are both open source licenses), so I was surprised to see it, but perhaps there is a good reason for it?

We need to add a version/revision table

Title of section
Change test case template to testing

Suggested section content and location
What should this new section contain, and where should it be stored in the table of contents?
Update and expand the test case template

Additional context
@Bai-Li-NOAA just added a PR to the FIMS repo which sets up the testing, so I think it would be good to describe how that is done in FIMS.

@Bai-Li-NOAA , would you like me to work on this? I would be happy to. Otherwise, I can be a second set of eyes on edits you make.

Choose and document a GitHub branching convention for the development of FIMS.

• log vs ln?
• tidyverse style guide?
• Google C++ style guide