Added coding standards for model implementations as a custom context for greptile #1219
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces comprehensive coding standards for model implementations and updates the multi-storage-client dependency across the project.
Main Changes:
- Created
CODING_STANDARDS/MODELS_IMPLEMENTATION.mddefining 4 strict rules (MOD-000 to MOD-003) covering model organization, class inheritance, lifecycle management, and documentation standards - Updated
greptile.jsonto reference the new coding standards as custom context for future AI-powered reviews - Bumped
multi-storage-clientfrom v0.14.0 to v0.33.0 acrossDockerfile,pyproject.toml, and example configurations - Added rust client configuration to
msc_config.yamlfor the new storage client version - Cleaned up documentation by removing roof crash example section and associated GIF file
Issues Found:
- The coding standards document contains 7 spelling/typo errors ("artchitectures", "andapplicable", "mnodel", "foward", "doctrsing", "te", "sepaarted")
MOD-003rule has incorrect title (duplicatesMOD-001title instead of "Model classes documentation")- Formatting inconsistency where the document demonstrates triple backticks for inline code when it requires double backticks
Confidence Score: 3/5
- This PR is safe to merge after fixing typos in the coding standards document
- Score reflects that the PR makes valuable additions (coding standards and dependency updates) with good intent and structure, but the coding standards document itself contains multiple typos and formatting errors that should be corrected before merge. The typos are in a standards document that will be used as authoritative reference for future code reviews, so accuracy is important. The dependency updates and configuration changes are correct and consistent across files.
- Pay close attention to
CODING_STANDARDS/MODELS_IMPLEMENTATION.md- it requires corrections for 7 typos, 1 title error, and 1 formatting inconsistency before serving as the authoritative coding standards reference
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| CODING_STANDARDS/MODELS_IMPLEMENTATION.md | 3/5 | New coding standards document with multiple spelling/typo errors and one formatting inconsistency that need correction |
| greptile.json | 5/5 | Updated custom context to reference the new coding standards file - configuration looks correct |
| Dockerfile | 5/5 | Bumped multi-storage-client version from 0.14.0 to 0.33.0 - consistent with other updates |
| pyproject.toml | 5/5 | Updated multi-storage-client dependency version to 0.33.0 - matches Dockerfile change |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant PR as Pull Request
participant Greptile as Greptile AI
participant Repo as Repository
Dev->>Repo: Create CODING_STANDARDS/MODELS_IMPLEMENTATION.md
Note over Dev,Repo: Add comprehensive model coding standards<br/>(MOD-000 to MOD-003)
Dev->>Repo: Update greptile.json
Note over Dev,Repo: Configure custom context<br/>to reference new standards file
Dev->>Repo: Update multi-storage-client v0.33.0
Note over Dev,Repo: Update Dockerfile, pyproject.toml,<br/>msc_config.yaml, requirements.txt
Dev->>Repo: Remove roof_crash.gif and README section
Dev->>PR: Submit PR #1219
PR->>Greptile: Trigger review
Greptile->>Repo: Read coding standards file
Greptile->>Repo: Validate greptile.json config
Greptile->>Repo: Check consistency across files
Greptile->>PR: Report typos in standards doc
Greptile->>PR: Generate review with comments
Note over Greptile,PR: Future model PRs will be reviewed<br/>against these standards automatically
8 files reviewed, 9 comments
Signed-off-by: Charlelie Laurent <[email protected]>
CharlelieLrt
force-pushed
the
greptile-coding-standards
branch
from
0a22285 to
87aa150
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds comprehensive coding standards for model implementations in PhysicsNeMo and configures Greptile to use them as custom context.
Key Changes:
- Created
CODING_STANDARDS/MODELS_IMPLEMENTATION.mddefining 4 rules (MOD-000 to MOD-003) covering model organization, inheritance, lifecycle, and documentation standards - Updated
greptile.jsonto reference the new standards document for AI-assisted code reviews - Standards require models to inherit from
physicsnemo.Module, follow NumPy docstring style with LaTeX math notation, and progress through experimental→production→deprecation lifecycle stages
Issues Found:
- Multiple typos in the documentation (already identified in previous comments)
- MOD-003 title incorrectly duplicates MOD-001's title - should be "Model classes documentation"
Confidence Score: 4/5
- This PR is safe to merge after fixing typos and the title duplication issue.
- The PR adds valuable documentation and configuration without modifying any production code. The only issues are cosmetic (typos and one title duplication), which have already been identified and need correction before merge.
- CODING_STANDARDS/MODELS_IMPLEMENTATION.md requires attention for typo corrections and the title duplication fix in MOD-003.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| CODING_STANDARDS/MODELS_IMPLEMENTATION.md | 4/5 | New coding standards document for model implementations with comprehensive rules. Contains several typos already flagged by previous comments, and one title duplication issue (MOD-003 has same title as MOD-001). |
| greptile.json | 5/5 | Configuration update to reference the new coding standards document for model implementations. The path and description are correctly specified. |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Grep as Greptile AI
participant Std as Coding Standards
participant PR as Pull Request
Dev->>PR: Create/Update model code
Dev->>PR: Submit PR for review
PR->>Grep: Trigger AI review
Grep->>Std: Load MODELS_IMPLEMENTATION.md
Std-->>Grep: Return rules (MOD-000 to MOD-003)
Grep->>PR: Analyze changed files
Grep->>Grep: Check inheritance (MOD-001)
Grep->>Grep: Check organization (MOD-000)
Grep->>Grep: Check lifecycle stage (MOD-002)
Grep->>Grep: Validate docstrings (MOD-003)
Grep->>PR: Post review comments
Dev->>PR: Address feedback
PR->>Grep: Re-review if configured
Grep->>PR: Final approval/comments
2 files reviewed, no comments
coreyjadams
left a comment
coreyjadams
left a comment
There was a problem hiding this comment.
Thanks for doing this. Can we consider two additional rules / guidelines for models?
- Production Testing structure. What tests are minimally required, what tests are encouraged? We should note that models can not move out of experimental without complete test coverage.
- Requirements. For models that introduce extra requirements, we should prescribe a method to note and track that. TBD on managing requirements in general so I'm not sure we need a solution now, but we should update it here when we get there.
| >>> model = SimpleEncoder(input_dim=784, latent_dim=64) | ||
| >>> x = torch.randn(32, 784) | ||
| >>> latent = model(x) | ||
| >>> latent.shape |
There was a problem hiding this comment.
We should have a comment in the guide about this. It's excellent to have; we could to note to users that code examples must run successfully since our CI checks it.
There was a problem hiding this comment.
Do you mean all models should have a runnable Examples section? For some we only have .. code-block::, would that be fine or do you think Examples is needed?
There was a problem hiding this comment.
I don't think every model needs a runnable example, what I meant was that this section - if present - will get tested by CI so it's worth a note in the guidelines about it. Something like ...
Your docstring is encouraged to have an example section to demonstrate basic construction and usage. Note that our CI system will automatically test these sections for correctness, when present.
I wasn't trying to force anything complicated here :)
|
@coreyjadams following up on these 2 things.
Right, I was planning on adding a rule for that.
What type of mechanism are you thinking of here? For example if something use apex and its not available we use a try/catch: try:
import apex
except ImportError:
OptionalDependencyFailure("apex")
apex = NoneIt's the pattern used in E2S for optional dependencies. What about cases where there is a fallback (e.g. many of apex layers can just fallback to torch layers in case apex is not available)? |
Signed-off-by: Charlelie Laurent <[email protected]>
Greptile OverviewGreptile SummaryThis PR adds comprehensive coding standards documentation for model implementations and configures Greptile to use these standards as context for AI-assisted code reviews. Key changes:
The coding standards document is well-structured with a quick-reference Rule Index and detailed explanations including rationale, examples, and anti-patterns. All previously identified typos have been addressed in the latest commit. Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant CS as CODING_STANDARDS/MODELS_IMPLEMENTATION.md
participant GR as greptile.json
participant AI as Greptile AI
Dev->>CS: Create coding standards document
Note over CS: Defines MOD-000 to MOD-003 rules<br/>for model implementations
Dev->>GR: Update custom context configuration
GR->>CS: References coding standards file
Note over GR: Adds path and description<br/>for AI agent context
AI->>GR: Load configuration
AI->>CS: Parse coding standards rules
Note over AI: Use rules when reviewing<br/>model-related code changes
|
|
re:requirements ... I'm not sure what the right solution is. Ideally, I want a system in physicsnemo that can do the following for us:
I just want to get to a state where we have consistent requirements to make installations easier for target models, if that makes sense, without bogging us all down in import paperwork more or less. I haven't actually got a plan for implementation, this is just daydreaming at the moment. |
Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
2 participants
PhysicsNeMo Pull Request
Description
Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.