Use new accordo APIs - #157
Conversation
**New Accordo API (clean abstraction):**
- New Accordo module structure with public API (__init__, config, result, snapshot, exceptions, validator)
- Internal implementation (_internal/codegen, hip_interop, ipc/communication)
- Snapshot-based caching: capture reference once, compare multiple optimized versions
- AccordoValidator with proper timeout handling and error detection
- KernelArg dataclass for cleaner argument specification
**Formula changes (preserves all main improvements):**
- Updated correctness_validation_pass() to use new Accordo API with caching
- Added write_and_log_optimized_code() to centralize iteration logging
- All formulas use write_and_log_optimized_code() for immediate LLM output capture
- find_kernel_file() already in main - formulas use it correctly
**Infrastructure:**
- Logger.save_iteration_code() for centralized iteration file management
- __main__.py passes trace_path to logger
- All examples/*.hip updated with hip_try macro for error handling
- .gitignore: added .rocprofv3/
**Preserved from main ("Unify counters report" #155):**
- OptimizationTracker helper methods (is_successful, get_best_code, etc.)
- Auto-success determination logic
- Proper tie-breaker with speedup
- _filter_compiler_errors() method
- Correct failure metrics (speedup 0.0 for build, 1.0 for correctness)
- Correct performance_validation flow (add_step first, query values, build report)
Clean API with everything under Accordo namespace:
- Accordo.Config (ValidationConfig)
- Accordo.KernelArg
- Accordo.Snapshot
- Accordo.Result (ValidationResult)
- Accordo.ArrayMismatch
- Accordo.Error (AccordoError)
- Accordo.BuildError (AccordoBuildError)
- Accordo.TimeoutError (AccordoTimeoutError)
- Accordo.ProcessError (AccordoProcessError)
- Accordo.ValidationError (AccordoValidationError)
Usage:
from accordo import Accordo
config = Accordo.Config(kernel_name="...", kernel_args=[...])
validator = Accordo(config)
result = validator.validate(...)
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Accordo validation system to improve efficiency and adds iteration code logging for better debugging. The key changes include:
- Refactored Accordo into a clean public API with snapshot-based validation caching
- Introduced snapshot caching to capture the reference application once and reuse it across multiple validations
- Added automatic iteration code logging to trace directory
- Enhanced example files with hip_try error handling macros
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/accordo/validator.py | New Accordo validator with snapshot-based API |
| src/accordo/config.py | Configuration classes for validation setup |
| src/accordo/snapshot.py | Snapshot data structure for cached validation |
| src/accordo/result.py | Validation result classes with detailed metrics |
| src/accordo/exceptions.py | Custom exception hierarchy for Accordo errors |
| src/accordo/init.py | Clean public API with nested class structure |
| src/accordo/_internal/ | Internal implementation modules (IPC, codegen, HIP interop) |
| src/intelliperf/formulas/formula_base.py | Snapshot caching in validation, new write_and_log_optimized_code method |
| src/intelliperf/formulas/memory_access.py | Uses new code logging method, reordered output fields |
| src/intelliperf/formulas/bank_conflict.py | Uses new code logging method, reordered output fields |
| src/intelliperf/formulas/atomic_contention.py | Uses new code logging method, reordered output fields |
| src/intelliperf/formulas/diagnose_only.py | Updated signature for API consistency |
| src/intelliperf/core/logger.py | New save_iteration_code method |
| src/intelliperf/main.py | Sets trace_path on logger for iteration logging |
| examples/*.hip | Added hip_try error handling macros |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| original_dir = os.getcwd() | ||
| try: | ||
| os.chdir(working_directory) | ||
| process_pid = os.posix_spawn(binary_cmd[0], binary_cmd, env) |
There was a problem hiding this comment.
The os.posix_spawn function spawns a process but there is no corresponding wait or cleanup after send_response. This can lead to zombie processes accumulating. After send_response(pipe_name) on line 373, add code to wait for the process to finish, such as os.waitpid(process_pid, 0).
| #define hip_try(expr) \ | ||
| do { \ | ||
| hipError_t err = (expr); \ | ||
| if (err != hipSuccess) { \ | ||
| const char* msg = hipGetErrorString(err); \ | ||
| throw std::runtime_error(std::string("HIP error: ") + msg); \ | ||
| } \ |
There was a problem hiding this comment.
[nitpick] The hip_try macro is defined inconsistently across example files. Most files use hipError_t err as the variable name, but examples/access_pattern/uncoalesced/uncoalesced.hip uses hipError_t status and includes FILE and LINE in the error message. Consider standardizing the macro definition across all examples for consistency and maintainability. The version with FILE and LINE provides better error diagnostics.
| #define hip_try(expr) \ | |
| do { \ | |
| hipError_t err = (expr); \ | |
| if (err != hipSuccess) { \ | |
| const char* msg = hipGetErrorString(err); \ | |
| throw std::runtime_error(std::string("HIP error: ") + msg); \ | |
| } \ | |
| #define hip_try(expr) \ | |
| do { \ | |
| hipError_t err = (expr); \ | |
| if (err != hipSuccess) { \ | |
| const char* msg = hipGetErrorString(err); \ | |
| throw std::runtime_error(std::string("HIP error at " + std::string(__FILE__) + ":" + \ | |
| std::to_string(__LINE__) + ": " + msg)); \ | |
| } \ |
| "optimization_history": self.optimization_tracker.to_dict(), | ||
| "formula": "memoryAccess", |
There was a problem hiding this comment.
[nitpick] The 'optimization_history' field has been moved before 'formula' in the output dictionary. This reordering changes the JSON output format which could break downstream consumers that rely on field ordering. While Python 3.7+ dictionaries maintain insertion order, changing the order of fields in API responses should be done deliberately as a breaking change.
| "optimization_history": self.optimization_tracker.to_dict(), | |
| "formula": "memoryAccess", | |
| "formula": "memoryAccess", | |
| "optimization_history": self.optimization_tracker.to_dict(), |
| return super().compile_pass() | ||
|
|
||
| def correctness_validation_pass(self): | ||
| def correctness_validation_pass(self, accordo_absolute_tolerance: float = 1e-6): |
There was a problem hiding this comment.
This method requires at most 2 positional arguments, whereas overridden Formula_Base.correctness_validation_pass requires at least 3.
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
No description provided.