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

Code cleanup, styling, and formatting #753

Merged
merged 62 commits into from Jan 16, 2022
Merged

Code cleanup, styling, and formatting #753

merged 62 commits into from Jan 16, 2022

Conversation

hsbadr
Copy link
Collaborator

@hsbadr hsbadr commented Jan 11, 2022

Close #752.

@dfalbel
Copy link
Member

dfalbel commented Jan 11, 2022

Looks great @hsbadr !

Just 2 comments:

  • Can we document how to run the linters/stylers for both C++ and R code in the CONTRIBUTING.md file?
  • Can we skip running the linters/stylers in the autogenerated files like: gen-*.R, lantern.h, lantern.cpp, RcppExports.R and RcppExports.cpp? Since they are often auto generated this would cause too large diffs for small changes.

@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 11, 2022

Looks great @hsbadr !

Just 2 comments:

  • Can we document how to run the linters/stylers for both C++ and R code in the CONTRIBUTING.md file?
  • Can we skip running the linters/stylers in the autogenerated files like: gen-*.R, lantern.h, lantern.cpp, RcppExports.R and RcppExports.cpp? Since they are often auto generated this would cause too large diffs for small changes.

Sure, I'll automate it in GHA.

Also, it seems that Window doesn't like that lantern.log is missing; we may check it does exist.

@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 11, 2022

  • Can we document how to run the linters/stylers for both C++ and R code in the CONTRIBUTING.md file?

@dfalbel GHA needs more work than this PR. So, I'd go with the documentation and update the workflow later. Here's the customized bash script for torch (needs: clang-format and styler):

Rscript -e 'styler::style_pkg(exclude_files = list.files("./R", pattern = "^gen-*.*|^RcppExports.*"))'
find . -type f \( -name 'DESCRIPTION' -o -name "*.R" \) ! -path "*/gen-*.*" ! -path "*/RcppExports.*" -exec sed -i -e 's/[ \t]*$//' {} \;
find . -type f \( -name '*.h' -o -name '*.hpp' -o -name '*.c' -o -name '*.cc' -o -name '*.cpp' -o -name '*.cxx' \) ! -path "*/gen-*.*" ! -path "*/lantern.*"  ! -path "*/RcppExports.*" -exec sed -i -e 's/[ \t]*$//' {} \;
find . -type f \( -name '*.h' -o -name '*.hpp' -o -name '*.c' -o -name '*.cc' -o -name '*.cpp' -o -name '*.cxx' \) ! -path "*/gen-*.*" ! -path "*/lantern.*"  ! -path "*/RcppExports.*" -exec clang-format -style=Google --verbose -i {} \;

The above lines will reproduce this PR. Would you prefer to run them on pull request or if github.event.head_commit.message contains 'format'... or just listing them in CONTRIBUTING.md? Where's that file? I can't find one in .github/.

@dfalbel
Copy link
Member

dfalbel commented Jan 11, 2022

Oh wow, we had one, but it seems that we have unintentionally removed at some point. Can't find when though :(
https://github.com/mlverse/torch/commits/ef92d035218f491e3fe736b4b59ee11ae665a384/.github/CONTRIBUTING.md
We can keep the documentation in this PR and once we recover the Contributing file we can add it to CONTRIBUTING.md.

I think running in PR's is great! What do you think?

Maybe we can also add this custom script to tools/style.sh so users can also quickly use it locally.

@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 11, 2022

Oh wow, we had one, but it seems that we have unintentionally removed at some point. Can't find when though :(
https://github.com/mlverse/torch/commits/ef92d035218f491e3fe736b4b59ee11ae665a384/.github/CONTRIBUTING.md
We can keep the documentation in this PR and once we recover the Contributing file we can add it to CONTRIBUTING.md.

Should I recover the latest version in this PR?

I think running in PR's is great! What do you think?

Yes, I usually run it on PR... but that commits format/style changes if needed. We can also run it on demand by adding [format] string in the commit message.

Maybe we can also add this custom script to tools/style.sh so users can also quickly use it locally.

Yeah, that makes sense and could be better that listing specific commands in CONTRIBUTING.md; we can just request running tools/style.sh before opening a PR. I'll create this script now.

@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 12, 2022

@dfalbel This is ready. I'll take a cut at GHA workflow later.

@dfalbel
Copy link
Member

dfalbel commented Jan 12, 2022

@hsbadr Thanks for getting the CONTRIBUTING guide back!

I'm trying to run the style.sh script and running into a state where I get a copy of all files but with a -e suffix.

image

Maybe I'm missing something?

tools/style.sh Outdated Show resolved Hide resolved
@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 12, 2022

I'm trying to run the style.sh script and running into a state where I get a copy of all files but with a -e suffix.

It seems that you use Posix Sed (BSD/macOS) not GNU Sed (Linux). I run the script on Linux (same in GHA).

hsbadr and others added 2 commits January 12, 2022 09:47
@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 12, 2022

Maybe I'm missing something?

@dfalbel Can you try now? I've replaced GNU sed with perl, for compatibility. Though, GNU sed can be installed on macOS too.

Copy link
Member

@dfalbel dfalbel left a comment

Choose a reason for hiding this comment

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

Thanks @hsbadr

Now I think formatting worked, but I don't completely reproduce the same changes in this PR. I get a large diff eg:
(Of course we can ignore the workflows change, but I would expect .R files and .cpp files to be identical)

❯ git diff --stat style 
 .github/CONTRIBUTING.md                            | 109 -------------------------------------------------------------------------------------------------------------
 .github/workflows/lantern.yaml                     |   2 +-
 .github/workflows/main.yaml                        |  38 ++++++++++++++++++--------------------
 .github/workflows/website.yaml                     |   2 +-
 NEWS.md                                            |   2 --
 R/autograd.R                                       |   6 ------
 R/device.R                                         |   1 -
 R/distributions-categorical.R                      |   1 -
 R/distributions-chi2.R                             |   1 -
 R/distributions-mixture_same_family.R              |   1 -
 R/distributions-multivariate_normal.R              |   1 -
 R/distributions-normal.R                           |   1 -
 R/gen-method.R                                     |   2 +-
 R/gen-namespace.R                                  |   4 ++--
 R/generator.R                                      |  10 ++++++----
 R/indexing.R                                       |   1 -
 R/jit-compile.R                                    |   1 -
 R/linalg.R                                         |  26 --------------------------
 R/nn-activation.R                                  |  25 -------------------------
 R/nn-batchnorm.R                                   |   3 ---
 R/nn-conv.R                                        |   5 -----
 R/nn-distance.R                                    |   1 -
 R/nn-dropout.R                                     |   3 ---
 R/nn-init.R                                        |  12 ------------
 R/nn-linear.R                                      |   3 ---
 R/nn-loss.R                                        |  12 ------------
 R/nn-normalization.R                               |   2 --
 R/nn-pooling.R                                     |  19 -------------------
 R/nn-rnn.R                                         |   3 ---
 R/nn-sparse.R                                      |   1 -
 R/nn-upsampling.R                                  |   1 -
 R/nn-utils-rnn.R                                   |   3 ---
 R/nn.R                                             |   3 ---
 R/nnf-activation.R                                 |   2 --
 R/optim.R                                          |   1 -
 R/quantization.R                                   |   1 +
 R/stack.R                                          |   1 -
 R/tensor.R                                         |  10 ----------
 R/trace.R                                          |   4 ----
 R/with-indices.R                                   |  10 ----------
 R/wrapers.R                                        |   1 -
 README.md                                          |  94 +++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------
 inst/include/lantern/types.h                       |  10 +++++-----
 inst/include/torch.h                               |   2 --
 inst/include/torch_types.h                         |   2 --
 lantern/include/lantern/types.h                    |  10 +++++-----
 lantern/src/Allocator.cpp                          |   2 --
 lantern/src/AllocatorCuda.cpp                      |   5 ++---
 lantern/src/Autograd.cpp                           |   4 ++--
 lantern/src/Backends.cpp                           |   4 ++--
 lantern/src/Compile.cpp                            |   1 -
 lantern/src/Contrib/SortVertices/cuda_utils.h      |   1 -
 lantern/src/Contrib/SortVertices/sort_vert.cpp     |   2 --
 lantern/src/Contrib/SortVertices/sort_vert_cpu.cpp |   1 -
 lantern/src/Contrib/Sparsemax.cpp                  |   2 --
 lantern/src/Delete.cpp                             |   3 ++-
 lantern/src/Device.cpp                             |   3 ++-
 lantern/src/Dimname.cpp                            |   3 ++-
 lantern/src/Dtype.cpp                              |   3 ++-
 lantern/src/Function.cpp                           |   1 -
 lantern/src/Function.h                             |   3 ++-
 lantern/src/Generator.cpp                          |   3 ++-
 lantern/src/IValue.cpp                             |   1 -
 lantern/src/Indexing.cpp                           |   3 ++-
 lantern/src/JITTypes.cpp                           |   2 --
 lantern/src/Layout.cpp                             |   3 ++-
 lantern/src/MemoryFormat.cpp                       |   3 ++-
 lantern/src/NNUtilsRnn.cpp                         |   3 ++-
 lantern/src/QScheme.cpp                            |   3 ++-
 lantern/src/Quantization.cpp                       |   3 ++-
 lantern/src/Reduction.cpp                          |   3 ++-
 lantern/src/Save.cpp                               |   2 +-
 lantern/src/Scalar.cpp                             |   3 ++-
 lantern/src/ScriptModule.cpp                       |   1 -
 lantern/src/Stack.cpp                              |   1 -
 lantern/src/Storage.cpp                            |   3 ++-
 lantern/src/Tensor.cpp                             |   3 ++-
 lantern/src/TensorList.cpp                         |   3 ++-
 lantern/src/TensorOptions.cpp                      |   3 ++-
 lantern/src/Threads.cpp                            |   1 -
 lantern/src/Trace.cpp                              |   1 -
 lantern/src/utils.cpp                              |   4 ++--
 lantern/tests/init.cpp                             |   4 ++--
 lantern/tests/main.cpp                             |   3 ++-

Maybe this was caused by a different version of styler or clang-format?

I have styler v1.6.2
clang-format version 8.0.0 (tags/google/stable/2019-01-18)

tools/style.sh Outdated Show resolved Hide resolved
@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 14, 2022

@dfalbel Note that this also fixes some GHA issues such as failed Rcpp build from source on macOS (due to a recent update that doesn't have binaries built yet), by installing all dependencies from binary for macOS and Windows, except versioned dependencies that may have old version binaries. This should also accelerate the workflow.

@dfalbel dfalbel added the win-gpu Trigger windows gpu CI label Jan 16, 2022
@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 16, 2022

@dfalbel Any idea?

No credentials detected, skipping authentication
Error: google-github-actions/setup-gcloud failed with: No credentials provided to export

@dfalbel dfalbel removed the win-gpu Trigger windows gpu CI label Jan 16, 2022
@dfalbel
Copy link
Member

dfalbel commented Jan 16, 2022

Oh I see, we can't run win-gpu tests on forked repos.

Fixes:
```
Warning: google-github-actions/setup-gcloud is pinned at HEAD. We strongly advise against pinning to "@master" as it may be unstable. Please update your GitHub Action YAML from:

    uses: 'google-github-actions/setup-gcloud@master'

to:

    uses: 'google-github-actions/setup-gcloud@v0'
```
@hsbadr
Copy link
Collaborator Author

hsbadr commented Jan 16, 2022

@dfalbel It seems that the GPU runs are stuck, after the win-gpu label:

Can't find any online and idle self-hosted runner in the current repository, account/organization that matches the required labels: 'self-hosted , linux'
Waiting for a self-hosted runner to pickup this job...

I think it's ready to go anyway.

@dfalbel dfalbel merged commit cd7bddc into mlverse:master Jan 16, 2022
@hsbadr hsbadr mentioned this pull request Jan 16, 2022
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.

Code cleanup, styling, and formatting
2 participants