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

Add support for @annotations #1439

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Conversation

WardBrian
Copy link
Member

Initial implementation of https://github.com/stan-dev/design-docs/blob/master/designs/0033-declaration-annotations.md:

  • Basic parsing of @annotations on declaration nodes, propagation of this information throughout compiler.
  • Implementation of @extern example from the design doc, can be placed on functions to silence the declared-but-undefined typechecking error function-by-function
  • Implementation of @silent, which allows a parameter which is not output by write_array. Particularly useful for custom transforms with jacobian+= where the "raw" form is not desired.
    Note: This feature does lead to models which prohibit certain features like standalone generated quantities.

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here:

Release notes

@extern can now be placed before a function declaration to mark it as defined in an external C++ file
@silent can be placed on a variable in one of the output blocks (e.g. parameters) to prevent it from appearing in the CSV output.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@WardBrian WardBrian added feature New feature or request parsing issues related to the parser and syntax errors labels Jul 15, 2024
A more targeted version of --allow-undefined
Removes item from output of write_array and related methods. Needs careful consideration on how this interacts with unconstrain_array function
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.48%. Comparing base (7865be8) to head (d14819d).

Files Patch % Lines
src/frontend/Typechecker.ml 62.50% 3 Missing ⚠️
src/frontend/Environment.ml 0.00% 1 Missing ⚠️
src/middle/Program.ml 66.66% 1 Missing ⚠️
src/middle/Stmt.ml 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
- Coverage   89.49%   89.48%   -0.02%     
==========================================
  Files          63       65       +2     
  Lines       10618    10642      +24     
==========================================
+ Hits         9503     9523      +20     
- Misses       1115     1119       +4     
Files Coverage Δ
src/analysis_and_optimization/Memory_patterns.ml 88.77% <ø> (ø)
src/analysis_and_optimization/Optimize.ml 92.25% <100.00%> (ø)
src/frontend/Annotations.ml 100.00% <100.00%> (ø)
src/frontend/Ast.ml 70.90% <100.00%> (+0.24%) ⬆️
src/frontend/Ast_to_Mir.ml 94.15% <100.00%> (ø)
src/frontend/Canonicalize.ml 96.36% <100.00%> (ø)
src/frontend/Pretty_printing.ml 92.36% <100.00%> (+0.08%) ⬆️
src/stan_math_backend/Annotations.ml 100.00% <100.00%> (ø)
src/stan_math_backend/Lower_functions.ml 98.22% <ø> (ø)
src/stan_math_backend/Lower_stmt.ml 95.65% <100.00%> (ø)
... and 6 more

@nhuurre
Copy link
Collaborator

nhuurre commented Jul 18, 2024

In the design doc discussion, @silent was the most controversial annotation; it was agreed that @gpu should be the first one.
I think it would be enough for the initial implementation of @gpu to check that the mem_pattern is SoA after optimization and emit a warning if not. Annotation-aware optimization passes can be added later.


I'd prefer formatting the annotation on its own line (like a Python function decorator)

@gpu
matrix[N,M] x;

This is more or less required for longer parametrized annotations, such as the hypothetical @extern("file.cpp" :: namespace).


Checking that the annotation belongs to a list of known annotations may be enough to catch typos, but you should also warn about inapplicable annotations (e.g. @silent on a function or non-toplevel variable, @extern on a function with a body)

@WardBrian
Copy link
Member Author

@gpu would make the most sense to be implemented 1) by someone other than myself who understands them more, and 2) as part of something like #1353. In particular, at the moment in the compiler there is a lot of legacy opencl stuff that does not interact with the SoA memory pattern things.

@silent was my motivation to getting around to actually implement this, since it seems to be a feature users ask for often (1, 2, 3, 4, stan-dev/cmdstan#553), and with the jacobian += changes encouraging more user-defined transforms I imagine it would be even more so. But it does have concerns with other features, so I would be willing to pull it into another PR for separate discussion

Formatting is currently set up with a hv box, so it will fit on one line if it can, but as soon as it can't, every annotation will appear on its own line. We could easily use a vbox instead if that's preferred -- I have a slight preference towards saving lines if they're short annotations

More advanced warnings are definitely something I want to look at before this becomes a non-draft; part of the issue is a lot of annotation logic will inherently live in the lower parts of the compiler, which currently never emit warnings, so wiring it up in a reasonable way requires some thinking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request parsing issues related to the parser and syntax errors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants