Skip to content

Run formatter, start using always_use_return = true #886

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mhauru
Copy link
Member

@mhauru mhauru commented Apr 10, 2025

JuliaFormatter has a new major release, and our GitHub actions are already using it. To catch up, I ran the formatter on all code locally. While I was at it, I enabled always_use_return = true, which according to the Blue style guide should be done anyway, but JuliaFormatter doesn't include it in its BlueStyle config (see domluna/JuliaFormatter.jl#906).

The first commit runs JuliaFormatter v2.1.0 with old settings and does nothing else. The second one removes trailing whitespace. The third adds always_use_return = true and its consequences.

Copy link
Contributor

Benchmark Report for Commit e366079

Computer Information

Julia Version 1.11.4
Commit 8561cc3d68d (2025-03-10 11:36 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × AMD EPYC 7763 64-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores)

Benchmark Results

|                 Model | Dimension |  AD Backend |      VarInfo Type | Linked | Eval Time / Ref Time | AD Time / Eval Time |
|-----------------------|-----------|-------------|-------------------|--------|----------------------|---------------------|
| Simple assume observe |         1 | forwarddiff |             typed |  false |                  9.7 |                 1.6 |
|           Smorgasbord |       201 | forwarddiff |             typed |  false |                613.9 |                43.3 |
|           Smorgasbord |       201 | forwarddiff | simple_namedtuple |   true |                436.9 |                44.7 |
|           Smorgasbord |       201 | forwarddiff |           untyped |   true |               1262.6 |                26.8 |
|           Smorgasbord |       201 | forwarddiff |       simple_dict |   true |               4243.6 |                18.8 |
|           Smorgasbord |       201 | reversediff |             typed |   true |               1498.2 |                29.4 |
|           Smorgasbord |       201 |    mooncake |             typed |   true |                967.6 |                 5.3 |
|    Loop univariate 1k |      1000 |    mooncake |             typed |   true |               5655.9 |                 4.1 |
|       Multivariate 1k |      1000 |    mooncake |             typed |   true |               1162.0 |                 8.2 |
|   Loop univariate 10k |     10000 |    mooncake |             typed |   true |              62951.3 |                 3.7 |
|      Multivariate 10k |     10000 |    mooncake |             typed |   true |               9775.3 |                 9.2 |
|               Dynamic |        10 |    mooncake |             typed |   true |                142.2 |                12.4 |
|              Submodel |         1 |    mooncake |             typed |   true |                 26.4 |                 8.6 |
|                   LDA |        12 | reversediff |             typed |   true |                490.4 |                 5.7 |

Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 87.30159% with 8 lines in your changes missing coverage. Please review.

Project coverage is 84.89%. Comparing base (019e41b) to head (e366079).

Files with missing lines Patch % Lines
src/varinfo.jl 82.85% 6 Missing ⚠️
src/model_utils.jl 0.00% 1 Missing ⚠️
src/simple_varinfo.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #886      +/-   ##
==========================================
+ Coverage   84.87%   84.89%   +0.01%     
==========================================
  Files          34       34              
  Lines        3815     3820       +5     
==========================================
+ Hits         3238     3243       +5     
  Misses        577      577              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coveralls
Copy link

coveralls commented Apr 10, 2025

Pull Request Test Coverage Report for Build 14377580780

Details

  • 27 of 63 (42.86%) changed or added relevant lines in 12 files are covered.
  • 1187 unchanged lines in 26 files lost coverage.
  • Overall coverage increased (+0.02%) to 84.984%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/model_utils.jl 0 1 0.0%
src/debug_utils.jl 0 2 0.0%
src/extract_priors.jl 0 2 0.0%
src/simple_varinfo.jl 1 3 33.33%
src/varinfo.jl 6 35 17.14%
Files with Coverage Reduction New Missed Lines %
src/DynamicPPL.jl 1 90.91%
src/submodel_macro.jl 2 82.35%
src/extract_priors.jl 4 51.72%
src/test_utils/model_interface.jl 5 22.22%
src/test_utils/varinfo.jl 5 78.26%
src/values_as_in_model.jl 6 43.9%
src/logdensityfunction.jl 7 65.22%
src/model_utils.jl 7 7.14%
src/distribution_wrappers.jl 13 9.76%
src/model.jl 13 80.33%
Totals Coverage Status
Change from base Build 14321484767: 0.02%
Covered Lines: 3243
Relevant Lines: 3816

💛 - Coveralls

@penelopeysm
Copy link
Member

penelopeysm commented Apr 10, 2025

Last time I checked, JuliaFormatter v2 had some bugs. (For additional context, the initial release of v2 was yanked because there were various bugs: JuliaRegistries/General#117438.) The issue tracker suggests to me that there are still quite a few outstanding issues. Are we sure that it's production ready?

Should we be fixing it to v1 instead? https://github.com/TuringLang/actions/blob/e94733241a8cb6a44159eff8eb347c1cb037a53c/Format/action.yml#L19-L25

In an ideal world, I'd love to use v2 and report issues upstream but I fear it might be too annoying to run and compare two different versions of JuliaFormatter.

@mhauru
Copy link
Member Author

mhauru commented Apr 10, 2025

That's a good question. v2 was fine on this repo, but I couldn't make a similar PR for Turing.jl because of a Formatter bug. Depends a lot on how quickly the bugs get fixed. I'm happy with fixing to v1 too.

@penelopeysm
Copy link
Member

Something like this is a bit scary: domluna/JuliaFormatter.jl#909

@penelopeysm
Copy link
Member

We've agreed to stick to v1 for now!

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.

3 participants