Onboard hyper into rush-py#96
Conversation
|
📖 Docs preview: https://talo.github.io/rush-py/pr-96/ |
There was a problem hiding this comment.
Standardize naming. Since this is a Python-submodule modules (as it should be, modeled after exess where there are multiple entrypoints to the same program), it should be like:
./src/rush/hyper/
__init__.py
_common.py (fine)
_optimize.py (unless minimization is distinct; we use optimize for exess and nnxtb)
_solvate.py
_simulate.py
Names should be the verb of the module, should be clear, should not be prefixed with the program name or suffixed with sumo, etc.
- Explode configs into the module function parameters: the top-level fields should be parameters, and if they themselves are objects, that's fine. So, there should be no generic
Configtype; if classes are needed for nested config values, don't call themConfig, call them something meaningful based on what they configure - In general, classes should be named without the module name prefixed, and "rex"/"sumo"/"tengu"/etc. should never be part of the name
- For Python-submodule modules, the individual entrypoints' classes should either be generically-named (if there's a "default" one) or all the classes should be uniformly prefixed with a meaningful name like
OptimizationInput,OptimizationRun,OptimizationResultRef,OptimizationResult,OptimizationResultPaths, etc. - "Run" should not be part of any of the type names either
- The module function itself should be named whatever you used for the prefix, or the verb-form of it if the case may be e.g.
optimize. Redundant naming like addinghyperto everything is unpythonic, namespaces and classes are widely used to scope names like that. - I very deliberately chose not to use
Outputin any of the class names; it's too similar toResultand not clear enough.
For batched runs, I think the best thing to do is to make ResultRef, Result, and ResultPaths the "single" version of the output, and have the module function return Run[Iterator[RunResultRef]]. I think that keep things sane and auto3d should be changed to work that way too. Then for fetch and save, we don't wanna support the error types as input. The user can easily filter them out, and it bloats the implementations. We don't have to do this for this PR, I'm just making a note here when thinking about the design.
I think there's probably more, but this is a start.
| stage: ErrorStage | ||
| category: ErrorCategory | ||
| message: str | ||
| input_index: int |
There was a problem hiding this comment.
Probably redundant and a bit sloppy? Maybe OK and doesn't hurt? 🤷
| input_index: int | ||
|
|
||
| @classmethod | ||
| def from_raw_output(cls, raw: Any) -> "ItemError": |
There was a problem hiding this comment.
| def from_raw_output(cls, raw: Any) -> "ItemError": | |
| def from_raw_output(cls, raw: Any) -> Self: |
| def _is_result_type(raw: Any) -> bool: | ||
| return isinstance(raw, dict) and len(raw) == 1 and ("Ok" in raw or "Err" in raw) |
There was a problem hiding this comment.
We should probably extract the inner function in runs.py and just reuse it.
| def _upload_json_object(input_object: JsonObjectInput) -> RushObject: | ||
| match input_object: | ||
| case RushObject(): | ||
| return input_object | ||
| case Path() | str() | dict(): | ||
| return RushObject.from_dict(upload_object(input_object)) | ||
| case _: | ||
| raise TypeError( | ||
| "Expected Path | str | RushObject | dict input for Hyper JSON object" | ||
| ) |
There was a problem hiding this comment.
Hrm I guess this should just be how rush.object.upload_object works...
There was a problem hiding this comment.
Actually, we should have upload_json and upload_bin as class methods on RushObject.
| def _to_rex_json_obj(obj: RushObject) -> str: | ||
| return ( | ||
| 'VirtualObject { path = "' | ||
| + str(obj.path) | ||
| + '", format = ObjectFormat::json, size = 0 }' | ||
| ) | ||
|
|
||
|
|
||
| def _format_rex_list(items: list[str]) -> str: | ||
| if not items: | ||
| return "[]" | ||
| return "[\n " + ",\n ".join(items) + "\n ]" |
There was a problem hiding this comment.
These could go in _rex.py probs
There was a problem hiding this comment.
And it should probably be _to_rex_obj and it should use the format in obj.
| def _save_run_artifact(obj: RushObject, ext: str, label: str) -> Path: | ||
| out_path = obj.save(ext=ext) | ||
| if obj.format.lower() == "json": | ||
| payload = _fetch_run_artifact_bytes(obj, label) | ||
| with out_path.open("wb") as out_file: | ||
| out_file.write(payload) | ||
| return out_path |
There was a problem hiding this comment.
This seems a bit like slop. Why is it saving and then opening it and writing to it again?
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class RunOutput: |
There was a problem hiding this comment.
| class RunOutput: | |
| class Result: |
Should match auto3d design
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class RunOutputPaths: |
There was a problem hiding this comment.
| class RunOutputPaths: | |
| class ResultPaths: |
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class TRCBatchResultRef: |
There was a problem hiding this comment.
Probably belongs in _common.py.
Then we could just create a TypeAlias to ResultRef if it's used as exactly a specific module's result reference.
| ) | ||
|
|
||
|
|
||
| def hyper_run_sumo( |
There was a problem hiding this comment.
| def hyper_run_sumo( | |
| def simulate( |
2 participants
Automated rush-py onboarding for
hyper.Module source: https://github.com/talo/tengu-hyper
Deploy paths:
hyper_minimize_sumo:github:talo/tengu-hyper/f2906f085c7a3a07104a98a263dc9876f08ee4e7#hyper_minimize_sumohyper_run_sumo:github:talo/tengu-hyper/f2906f085c7a3a07104a98a263dc9876f08ee4e7#hyper_run_sumohyper_solvate_sumo:github:talo/tengu-hyper/f2906f085c7a3a07104a98a263dc9876f08ee4e7#hyper_solvate_sumo