-
Notifications
You must be signed in to change notification settings - Fork 14
chore: Improve agents.md
#598
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,185 @@ | ||
| - Only generate docstrings for public functions or functions that contain more than 4 lines of code. | ||
| - Use the Sphinx style for Python docstrings (e.g. :param my_param: Does something) and never | ||
| include a :returns: key. | ||
| - The code you generate should always contain type hints in the function prototypes (including | ||
| return type hints of None), but type hints are not needed when initializing variables. | ||
| - We use uv for everything (e.g. we do `uv run python ...` to run some python code, and | ||
| `uv run pytest tests/unit` to run unit tests). Please prefer `uv run python -c ...` over | ||
| `python3 -c ...` | ||
| - When you create or modify a code example in a public docstring, always update the corresponding | ||
| doc test in the appropriate file of `tests/doc`. This also applies to any change in an example of | ||
| a `.rst` file, that must be updated in the corresponding test in `tests/doc/test_rst.py`. | ||
| - After generating code, please run `uv run ty check`, `uv run ruff check` and `uv run ruff format`. | ||
| Fix any error. | ||
| - After changing anything in `src` or in `tests/unit` or `tests/doc`, please identify the affected | ||
| test files in `tests/` and run them with e.g. | ||
| `uv run pytest tests/unit/aggregation/test_upgrad.py -W error`. Fix any error, either in the | ||
| changes you've made or by adapting the tests to the new specifications. | ||
| # AGENTS.md - TorchJD Development Guide | ||
|
|
||
| This file contains guidelines for agentic coding agents working in this repository. | ||
|
|
||
| ## Running Commands | ||
|
|
||
| ### Using uv | ||
| We use [uv](https://docs.astral.sh/uv/) for all Python operations. Always use `uv run` prefix: | ||
| ```bash | ||
| uv run python ... # Run Python code | ||
| uv run pytest ... # Run tests | ||
| uv run ty check ... # Type checking | ||
| uv run ruff check ... # Linting | ||
| uv run ruff format ... # Formatting | ||
| ``` | ||
|
|
||
| ### Running Tests | ||
|
|
||
| Run all unit tests: | ||
| ```bash | ||
| uv run pytest tests/unit | ||
| ``` | ||
|
|
||
| Run a single test file: | ||
| ```bash | ||
| uv run pytest tests/unit/aggregation/test_upgrad.py -W error | ||
| ``` | ||
|
|
||
| Run a single test function: | ||
| ```bash | ||
| uv run pytest tests/unit/aggregation/test_upgrad.py::test_function_name -W error | ||
| ``` | ||
|
|
||
| Run tests with slow tests included: | ||
| ```bash | ||
| uv run pytest tests/unit --runslow | ||
| ``` | ||
|
|
||
| Run doc tests (examples in docstrings and .rst files): | ||
| ```bash | ||
| uv run pytest tests/doc | ||
| ``` | ||
|
|
||
| Run tests on CUDA (requires GPU): | ||
| ```bash | ||
| CUBLAS_WORKSPACE_CONFIG=:4096:8 PYTEST_TORCH_DEVICE=cuda:0 uv run pytest tests/unit | ||
| ``` | ||
|
|
||
| Run tests with coverage: | ||
| ```bash | ||
| uv run pytest tests/unit tests/doc --cov=src | ||
| ``` | ||
|
Comment on lines
+49
to
+52
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really necessary for the agent to check code coverage every time. Also, it should already know how to use the --cov option from pytest. |
||
|
|
||
| ### Linting and Type Checking | ||
|
|
||
| Run type checking: | ||
| ```bash | ||
| uv run ty check | ||
| ``` | ||
|
|
||
| Run linting: | ||
| ```bash | ||
| uv run ruff check | ||
| ``` | ||
|
|
||
| Run formatting: | ||
| ```bash | ||
| uv run ruff format | ||
| ``` | ||
|
|
||
| Run all checks together: | ||
| ```bash | ||
| uv run ty check && uv run ruff check && uv run ruff format | ||
| ``` | ||
|
Comment on lines
+54
to
+74
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably remove all of that and just add that as an agent post-completion hook. Whatever agent provider we use in the future should also support hooks IMO. |
||
|
|
||
| ### Building Documentation | ||
|
|
||
| From the `docs` folder: | ||
| ```bash | ||
| uv run make html | ||
| ``` | ||
|
|
||
| Clean build: | ||
| ```bash | ||
| uv run make clean | ||
| ``` | ||
|
Comment on lines
+78
to
+86
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should merge those two to gain tokens. |
||
|
|
||
| ## Code Style Guidelines | ||
|
|
||
| ### Docstrings | ||
|
|
||
| - Only generate docstrings for public functions or functions with more than 4 lines of code | ||
| - Use Sphinx style (`:param my_param: Does something`) - never use `:returns:` key | ||
| - Include usage examples in docstrings for public classes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we want agents to do the usage examples themselves. |
||
| - Always update corresponding doc tests in `tests/doc/` when modifying examples | ||
|
|
||
| ### Type Hints | ||
|
|
||
| - Always include type hints in function prototypes (including `-> None`) | ||
| - Do not include type hints when initializing local variables | ||
| - Use `ty` for type checking | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agents shouldn't even know we use ty for type checking, as part of the text we send to them. This should be a hook IMO. |
||
|
|
||
| ### Imports | ||
|
|
||
| - Use `combine-as-imports = true` (configured in pyproject.toml) | ||
| - Order imports: standard library, third-party, local | ||
| - Use absolute imports (e.g., `from torchjd.aggregation import UPGrad`) | ||
|
Comment on lines
+103
to
+107
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for that IMO. With the hooks, this will be autofixed. Also we don't really have a rule about absolute imports. We even try to use relative imports in many cases. |
||
|
|
||
| ### Formatting | ||
|
|
||
| - Line length: 100 characters (enforced by ruff) | ||
| - Quote style: double quotes | ||
| - Run `uv run ruff format` before committing | ||
|
Comment on lines
+109
to
+113
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can remove all of that if we use hook. |
||
|
|
||
| ### Naming Conventions | ||
|
|
||
| - Classes: `PascalCase` (e.g., `class UPGrad`) | ||
| - Functions/methods: `snake_case` (e.g., `def jac_to_grad`) | ||
| - Constants: `UPPER_SNAKE_CASE` | ||
|
Comment on lines
+115
to
+119
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first 3 points are standard in python so the agents will already do that by default. |
||
| - Private functions: prefix with `_` (e.g., `_internal_func`) | ||
| - Private modules: prefix with `_` (e.g., `_utils`) | ||
|
|
||
| ### Error Handling | ||
|
|
||
| - Use appropriate exception types from Python standard library or PyTorch | ||
| - Provide clear error messages with context | ||
| - Validate inputs at function boundaries | ||
| - Use assertions for internal invariants | ||
|
Comment on lines
+123
to
+128
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's quite standard, so that's probably something the agent will already do by default (it may even be written in the system prompt of the agent, or something similar). |
||
|
|
||
| ### Testing | ||
|
|
||
| - Use partial tensor functions from `tests/utils/tensors.py` (e.g., `ones_()`, `randn_()`) | ||
| - This ensures tensors are on correct device and dtype automatically | ||
| - Manually move models/rng to DEVICE (defined in `settings.py`) | ||
| - Use `ModuleFactory` for creating modules on correct device | ||
| - Mark slow tests with `@pytest.mark.slow` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. slow tests are very niche, I wouldn't even talk about them to the agent. |
||
| - Run affected tests after changes: `uv run pytest tests/unit/<module> -W error` | ||
|
|
||
| ### Project Structure | ||
|
|
||
| ``` | ||
| src/torchjd/ | ||
| ├── __init__.py # Public API exports | ||
| ├── autojac/ # Jacobian computation engine | ||
| │ ├── _backward.py | ||
| │ ├── _jac.py | ||
| │ ├── _jac_to_grad.py | ||
| │ ├── _mtl_backward.py | ||
| │ └── _transform/ # Internal transform implementations | ||
| ├── autogram/ # Gramian-based engine | ||
| │ ├── _engine.py | ||
| │ └── ... | ||
| ├── aggregation/ # Aggregators and weightings | ||
| │ ├── _upgrad.py | ||
| │ ├── _mean.py | ||
| │ └── _utils/ # Internal utilities | ||
| └── _linalg/ # Linear algebra utilities | ||
|
|
||
| tests/ | ||
| ├── unit/ # Unit tests (mirrors src structure) | ||
| ├── doc/ # Docstring and rst example tests | ||
| └── utils/ # Test utilities (tensors.py, settings.py) | ||
| ``` | ||
|
Comment on lines
+139
to
+163
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's gonna be a pain to maintain. Also the agent is able to run a tree command to get this information when it needs, or we could force it to get this information using a pre-hook that runs |
||
|
|
||
| ### Adding New Aggregators | ||
|
|
||
| - Subclass `Aggregator` or `Weighting` base classes | ||
| - Aggregators must be **immutable** (no stateful changes) | ||
| - Implement the mathematical mapping as documented | ||
| - Add corresponding weighting if applicable | ||
| - Update `__init__.py` to export the new class | ||
|
Comment on lines
+165
to
+171
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's directed to individual contributors. I don't think this is worth the tokens in agents.md. |
||
|
|
||
| ### Deprecation | ||
|
|
||
| - Raise `DeprecationWarning` for deprecated public functionality | ||
| - Add test in `tests/unit/test_deprecations.py` to verify warning | ||
|
Comment on lines
+173
to
+176
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is extremely niche IMO. This information is mostly directed to maintainers. |
||
|
|
||
| ### Code Quality | ||
|
|
||
| - Follow SOLID principles | ||
| - Keep code simple and readable | ||
| - Prefer explicit over implicit | ||
| - Add type hints to all public interfaces | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already said somewhere. |
||
| - Document complex algorithms with comments | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LLMs tend to already overuse comments, so I don't think that's needed. |
||
| - Run all checks before submitting: `uv run ty check && uv run ruff check && uv run ruff format` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hook |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will confuse the agent whenever it runs on an environment without gpu access.