Implement Claude Skills for Hyrax development workflows#659
Implement Claude Skills for Hyrax development workflows#659Copilot wants to merge 7 commits intomtauraso/ultimate-robot-instructionsfrom
Conversation
Implement four Claude skills as requested: 1. Hyrax Development Workflow - setup, validation, timeouts 2. Hyrax Testing Strategy - pytest markers, fixtures, parallel execution 3. Adding Hyrax Components - models, datasets, verbs with decorators 4. Hyrax Configuration System - ConfigDict, defaults, immutability Skills are JSON files in .github/skills/ with structured guidance. Updated CLAUDE.md to reference the new skills. Co-authored-by: mtauraso <31012+mtauraso@users.noreply.github.com>
The four JSON skill files were initially blocked by .gitignore's *.json pattern. Force-adding them as they are configuration/documentation files, not data files. Co-authored-by: mtauraso <31012+mtauraso@users.noreply.github.com>
Add exception to .gitignore's *.json pattern to allow tracking .github/skills/*.json files, which are configuration/documentation rather than data files. Co-authored-by: mtauraso <31012+mtauraso@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Please take these same skills and put them into .md files, removing the .json files. Follow the standard practice in https://code.claude.com/docs/en/skills and https://docs.github.com/en/copilot/concepts/agents/about-agent-skills where all the skills are .md files |
Following standard practice from Claude Code and GitHub Copilot documentation, convert all skill files from .json to .md format with YAML frontmatter. Changes: - Convert 4 JSON skills to Markdown with YAML frontmatter - Remove .json files - Update CLAUDE.md links to reference .md files - Update .github/skills/README.md to reflect Markdown format - Revert .gitignore exception (no longer needed for .md files) Co-authored-by: mtauraso <31012+mtauraso@users.noreply.github.com>
Converted all Claude skills from JSON to Markdown format following standard practice from Claude Code and GitHub Copilot documentation. Changes made:
All skills now use Markdown format with frontmatter containing name, description, version, and tags metadata. Commit: 6f329cf |
| ### Dataset Requirements | ||
|
|
||
| All Hyrax datasets MUST: | ||
| 1. Subclass `HyraxDataset` or `HyraxImageDataset` |
There was a problem hiding this comment.
Correction:
- Subclass
HyraxDatasetdirectly or indirectly through child classes
| with open(split_file) as f: | ||
| return [line.strip() for line in f] | ||
|
|
||
| def _load_image(self, path): |
There was a problem hiding this comment.
Datasets don't necessarily have images
| "metadata": self._get_metadata(idx) | ||
| } | ||
|
|
||
| def _load_file_list(self, split): |
There was a problem hiding this comment.
Datasets don't necessarily have a list of files
| image_path = self.file_list[idx] | ||
| image = self._load_image(image_path) | ||
|
|
||
| # Apply transforms (handled by HyraxImageDataset) |
There was a problem hiding this comment.
Datasets don't necessarily have an image or transformations
| from pathlib import Path | ||
| from hyrax.data_sets.hyrax_image_dataset import HyraxImageDataset | ||
|
|
||
| class MyDataset(HyraxImageDataset): |
There was a problem hiding this comment.
For this example, it should be simpler and remove image-specific code. Some datasets are tabular, and that is a simpler and more general case.
| # Return label if supervised, None if unsupervised | ||
| pass | ||
|
|
||
| def _get_metadata(self, idx): |
There was a problem hiding this comment.
The real metadata interface defined in HyraxDataset is omitted here and should be represented.
This internal member should not be here.
| ```toml | ||
| [data] | ||
| name = "MyDataset" | ||
| root_dir = "/path/to/data" |
There was a problem hiding this comment.
data_location is preferred by convention here
| All Hyrax verbs MUST: | ||
| 1. Use `@hyrax_verb("verb_name")` decorator | ||
| 2. Implement `run()` and `run_cli()` methods | ||
| 3. Implement `setup_parser(parser)` for CLI args |
There was a problem hiding this comment.
Most verbs shouldn't have arguments or take parameters. Strongly prefer configs to parameters.
| Args: | ||
| parser: argparse.ArgumentParser | ||
| """ | ||
| parser.add_argument( |
There was a problem hiding this comment.
Example verb should have no CLI args or parameters
| hyrax myverb -c config.toml -o output/ --option | ||
| ``` | ||
|
|
||
| ## Common Registration Issues |
There was a problem hiding this comment.
Remove this section. We will add it in if needed
| 4. Check `add_parser_kwargs` is set | ||
| 5. Verify no syntax errors in verb file | ||
|
|
||
| ## External Plugins |
There was a problem hiding this comment.
Remove this section. External plugins may be their own topic in future. This is out of scope
| value = config["nonexistent_key"] # KeyError | ||
| ``` | ||
|
|
||
| **ConfigDict** (USE THIS): |
There was a problem hiding this comment.
ConfigDict is internal to the config system, so it shouldn't really get created outside of there, which means this example and a lot of the others are wrong.
Overall make this much shorter emphasizing the invariants of the config system over precise usage
|
@copilot Please address inline comments |
Changes to adding-hyrax-components.md: - Clarify datasets can subclass HyraxDataset directly or indirectly - Replace image-specific example with simpler tabular dataset example - Remove image-specific code (transforms, file lists, etc.) - Use data_location convention instead of root_dir - Simplify verb example to remove CLI args/parameters (prefer config) - Remove "Common Registration Issues" section (out of scope) - Remove "External Plugins" section (out of scope) Changes to hyrax-configuration-system.md: - Significantly shortened to emphasize invariants over implementation - Removed ConfigDict creation examples (internal to config system) - Focus on accessing config via hyrax.config, not creating directly - Removed verbose sections on Pydantic, hierarchy, debugging Changes to hyrax-testing-strategy.md: - Add warning about slow tests on GitHub Actions (will timeout) - Clarify to only run slow tests on beefy local hardware Co-authored-by: mtauraso <31012+mtauraso@users.noreply.github.com>
Addressed all inline comments: adding-hyrax-components.md:
hyrax-configuration-system.md:
hyrax-testing-strategy.md:
Commit: 8e32571 |
|
|
||
| ## The `key = false` Convention | ||
|
|
||
| **Convention**: `key = false` in TOML means the key is optional and evaluates to `None` in Python. |
There was a problem hiding this comment.
It doesn't evaluate to None in Python. We in the Hyrax project commonly use key=false in toml to mean key=None, because toml has no way to assign None to a key syntactically.
The common codebase pattern that creates this social convention is
foo_value = config['foo'] if config['foo'] != False else None
key=false in default TOML means the key is either:
- Required to run, and the user must supply it
- Optional, but the
nonevalue means something sensible (e.g. feature disabled)
| ## Best Practices | ||
|
|
||
| 1. **Always provide defaults**: Every key must be in `hyrax_default_config.toml` | ||
| 2. **Use `key = false` for optional features**: This clearly indicates optional parameters |
There was a problem hiding this comment.
Use key=false convention in situations where the config is required to be specified by the user, or where lacking the config overall has behavior that would make sense to the user.
| """Return the first record as a sample.""" | ||
| return {"data": self.data[0]} | ||
|
|
||
| @classmethod |
| # e.g., np.load(), pd.read_csv(), etc. | ||
| pass | ||
|
|
||
| def sample_data(self): |
| # Implementation depends on data format | ||
| # e.g., np.load(), pd.read_csv(), etc. | ||
| pass | ||
|
|
There was a problem hiding this comment.
Where is the metadata interface in this example?
|
@copilot Please edit these skills to address all unresolved comments |
Configuration system (hyrax-configuration-system.md): - Corrected key=false convention explanation - it represents None in Python - Added common codebase pattern: config['foo'] != False check - Clarified when to use key=false (required input or optional with None behavior) - Updated best practices to reflect correct usage Adding components (adding-hyrax-components.md): - Removed optional sample_data() and is_map() methods from example - Removed label handling from __getitem__ (not in minimal example) - Added section on optional metadata interface with Astropy Table - Shows how to pass metadata_table to super().__init__() for visualization Co-authored-by: mtauraso <31012+mtauraso@users.noreply.github.com>
Addressed all unresolved inline comments: hyrax-configuration-system.md:
adding-hyrax-components.md:
Commit: a7e1bef |
|
I'm giving up on this and closing this out, because the quality isn't converging. |
Change Description
Implements four Claude Code skills (reusable task patterns) to provide structured guidance for common Hyrax development workflows. Addresses feedback from PRs #656 and #635 that robots were getting "bogged down with the amount of dev tooling" and that "many of these sections feel like they should be claude skills."
Solution Description
Claude Skills Created
Four Markdown skills in
.github/skills/:key=falseconvention (correctly explained), immutability rulesEach skill provides:
Skills follow standard practice from Claude Code and GitHub Copilot documentation using Markdown format with YAML frontmatter.
Key Design Decisions
Simplicity and Accuracy:
key=falseconvention as Hyrax project pattern (TOML has no None syntax)config['foo'] if config['foo'] != False else NoneHardware Awareness:
Integration
CLAUDE.mdwith skills section linking to all four skills.github/skills/README.mdexplaining skill usage and structure.mdfiles that work with both Claude Code and GitHub CopilotRationale
Skills address specific pain points identified in feedback:
key=falseconvention)Code Quality
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.