Add instructions against common AI poor code quality habits#718
Add instructions against common AI poor code quality habits#718jaredoconnell wants to merge 1 commit intovllm-project:mainfrom
Conversation
Signed-off-by: Jared O'Connell <joconnel@redhat.com>
dbutenhof
left a comment
There was a problem hiding this comment.
I like the idea of accumulating protections and advice as we go, so this is a fantastic precedent. We need to be cautious about the wording, so I'm expressing some concerns.
| - All tests must pass before committing | ||
| - Markdown files must be properly formatted | ||
| - Public functions in `src/` code must use the reStructuredText docstring format | ||
| - All imports must be done at the top of the file unless necessary for functionality |
There was a problem hiding this comment.
Can we be more specific? I think the only time we legitimately do that is to narrow the scope of optional extra packages; so maybe even go very specific like "unless necessary to minimize the scope of optional imports listed under the pyproject.toml "project.optional-dependencies" section."
There was a problem hiding this comment.
I would make this more broad, not more specific. If you give AI an excuse to ignore a rule it will twist its logic to match.
| - All imports must be done at the top of the file unless necessary for functionality | |
| - All imports **SHALL** be done at the top of the file |
| - Markdown files must be properly formatted | ||
| - Public functions in `src/` code must use the reStructuredText docstring format | ||
| - All imports must be done at the top of the file unless necessary for functionality | ||
| - Use of `getattr` should be avoided if possible as it hides incorrect usage of types |
There was a problem hiding this comment.
We use getattr quite a bit -- again, I wonder if we can be more specific here. I kinda worry about how an over-eager-to-please LLM might choose to interpret "if possible". I'm not really coming up with a better condition right now, but this feels like a plot hole ... 😆
There was a problem hiding this comment.
This is general guidance for AIs, not humans. If a human wants to override this guidance for a good reason they are free to do so.
| - Use of `getattr` should be avoided if possible as it hides incorrect usage of types | |
| - **DO NOT** use `getattr` or `setattr` as it hides incorrect usage of types |
There was a problem hiding this comment.
My concern is that an over-eager AI assistant will notice & decide to "fix" existing instances. Ah well -- we can burn that bridge when we reach it.
sjmonson
left a comment
There was a problem hiding this comment.
Please restructure the sections a bit as we will probably add more guidance in the future.
### Quality Requirements
- All Python code must pass linting and formatting
- All Python code must pass type checking
- All tests must pass before committing
- Markdown files must be properly formatted
### Style Requirements
- Public functions in `src/` code must use the reStructuredText docstring format
- All imports must be done at the top of the file unless necessary for functionality
- Use of `getattr` should be avoided if possible as it hides incorrect usage of typesAlso see wording suggestions below.
| - All tests must pass before committing | ||
| - Markdown files must be properly formatted | ||
| - Public functions in `src/` code must use the reStructuredText docstring format | ||
| - All imports must be done at the top of the file unless necessary for functionality |
There was a problem hiding this comment.
I would make this more broad, not more specific. If you give AI an excuse to ignore a rule it will twist its logic to match.
| - All imports must be done at the top of the file unless necessary for functionality | |
| - All imports **SHALL** be done at the top of the file |
| - Markdown files must be properly formatted | ||
| - Public functions in `src/` code must use the reStructuredText docstring format | ||
| - All imports must be done at the top of the file unless necessary for functionality | ||
| - Use of `getattr` should be avoided if possible as it hides incorrect usage of types |
There was a problem hiding this comment.
This is general guidance for AIs, not humans. If a human wants to override this guidance for a good reason they are free to do so.
| - Use of `getattr` should be avoided if possible as it hides incorrect usage of types | |
| - **DO NOT** use `getattr` or `setattr` as it hides incorrect usage of types |
dbutenhof
left a comment
There was a problem hiding this comment.
I've been playing with my GitHub PM tool, and I added an AGENTS.md to be sure it checked & fixed all formatting & tests before "finishing". I found it's lazy about keeping the rules in context. It suggested adding a section at the beginning, and that seems to be working (so far), so we should consider something like this:
## Read this file when starting or resuming work
- **Open `AGENTS.md` again** when you begin a task on this repo or return after a long gap, so required checks (below) stay in context until all required checks are green.| - Markdown files must be properly formatted | ||
| - Public functions in `src/` code must use the reStructuredText docstring format | ||
| - All imports must be done at the top of the file unless necessary for functionality | ||
| - Use of `getattr` should be avoided if possible as it hides incorrect usage of types |
There was a problem hiding this comment.
My concern is that an over-eager AI assistant will notice & decide to "fix" existing instances. Ah well -- we can burn that bridge when we reach it.
Summary
AI bots very frequently tend to import things inline and use
getattr. But we have always found that to be a bad design, so this documents AI chatbots not to do these things.Details
Use of AI
## WRITTEN BY AI ##)