feat(hosting): shared engineered-features module + model package export - #40
feat(hosting): shared engineered-features module + model package export#40helder-moreira wants to merge 17 commits into
Conversation
7b22bcd to
afbd1f7
Compare
helder-moreira
left a comment
helder-moreira
left a comment
There was a problem hiding this comment.
Multi-Agent Review — APPROVE
Panel: GPT-5.6 Sol · GLM 5.2 · Kimi K2.7 Code — all 3 reviewers returned APPROVE.
The shared engineered_features module is a clean, correct single source of truth for log-return features. The new vectorized computation is mathematically identical to the old inline engineer_returns: closes[-1] vs closes[-(w+1)] maps exactly to feature_close_{N-1} vs feature_close_{N-1-w}, and the guard conditions match. Both walkthrough .py files and the notebook now import the module in train and predict paths with the same ENGINEERED_SPECS, and all 6 parity tests pass.
Column naming change from log_return_1h→log_return_1 is intentional and correct: window_bars is interval-agnostic, so the same module works for 1h bars (topic 69) and 5m bars (topic 77) without hardcoded time-unit suffixes.
Findings
All findings are nits/suggestions — nothing blocking.
(Reviewed by Helder's Agent — GPT-5.6 Sol + GLM 5.2 + Kimi K2.7 Code)
…ing Phase 0) Extract the multi-horizon log-return features into apply_engineered_features so training and serving compute them identically (no train/serve skew); refactor the walkthroughs + notebook to use it; revive the parity test scoped to engineered features.
|
Addressed review: switched the unsupported-kind test to |
afbd1f7 to
f072aa7
Compare
helder-moreira
left a comment
helder-moreira
left a comment
There was a problem hiding this comment.
Re-Review — APPROVE
Panel: GPT-5.6 Sol · GLM 5.2 · DeepSeek V4 Pro — all 3 returned APPROVE.
Both prior nits are resolved:
pytest.raises(ValueError, match="unsupported")now used in rejection tests ✅- Dedicated
test_engineered_feature_names_rejects_unsupportedadded, covering theengineered_feature_namescode path ✅
All 7 parity tests pass. No new issues introduced by the force-push.
(Reviewed by Helder's Agent — GPT-5.6 Sol + GLM 5.2 + DeepSeek V4 Pro)
helder-moreira
left a comment
helder-moreira
left a comment
There was a problem hiding this comment.
Final Review — APPROVE
Panel: GPT-5.6 Sol · DeepSeek V4 Pro · GLM 5.2 — unanimous APPROVE.
Third sign-off pass confirms merge-readiness. The shared engineered_features module correctly eliminates train/serve skew by routing both paths through apply_engineered_features with identical specs. All 7 unit tests pass (name stability, train==serve parity, reference computation, zero-fill guards, non-mutation, error handling). Both prior nits from round 1 remain fixed. No new commits since prior APPROVE — HEAD f072aa7 unchanged.
Verdict: Merge-ready.
(Reviewed by Helder's Agent — GPT-5.6 Sol + DeepSeek V4 Pro + GLM 5.2)
helder-moreira
changed the title
03de0d4 to
014b152
Compare
helder-moreira
changed the title
03de0d4 to
898395a
Compare
helder-moreira
changed the title
898395a to
3cf0b8c
Compare
Add `export_package` / `allora-forge-export` CLI that turns a builder-kit model into an uploadable hosting package: generic env-driven model.py (PAIR/TIMEFRAME from env, so one package deploys to many pairs/timeframes), config.json, pyproject (SDK unpinned, builder-kit git ref, entry-point), manifest.json, .dockerignore, and optional bundled weights/. Both train-on-platform and train-locally supported.
3cf0b8c to
11e1871
Compare
helder-moreira
left a comment
helder-moreira
left a comment
There was a problem hiding this comment.
Review — APPROVE (with warning)
Panel: GPT-5.6 Sol (REQUEST_CHANGES — stale-file re-export) · GLM 5.2 (APPROVE) · Kimi K3 (APPROVE) · Claude Opus (pending)
The shared engineered-features module and parity tests remain sound (3 prior APPROVE rounds). The new package export feature is well-structured: ModelSpec validation, export_package() generating model.py/config.json/pyproject.toml/manifest.json, correct train/serve parity via apply_engineered_features, 21 tests passing.
Non-blocking warning (downgraded from GPT-5.6's critical)
Stale files on re-export to the same directory (export.py:139): If export_package is called with weights_dir and then re-called without it on the same out_dir, the stale weights/ directory remains while manifest.json reports has_weights: false. This is a consistency issue, not a correctness or security bug — the export function is a CLI tool typically run once per package, not iteratively. Consider exporting into a clean/staged directory or clearing out_dir before writing.
Parent-verification note
GPT-5.6 Sol cited line 239, but the actual stale-file concern is in the export_package function at lines 121+139. Line 239 is inside the _MODEL_PY template string. The finding is valid; the line number was inaccurate.
(Reviewed by Helder's Agent — GPT-5.6 Sol + GLM 5.2 + Kimi K3 + Claude Opus)
Claude Opus 4.8 Review — APPROVEThe new single-package export feature is clean, well-documented, and thoroughly tested (21 tests pass locally). It correctly bakes model-intrinsic config into [suggestion] [nit] (Claude Opus 4.8 via CLI — 4th reviewer) |
helder-moreira
left a comment
helder-moreira
left a comment
There was a problem hiding this comment.
Corrected Aggregated Review — COMMENT
Panel: GPT-5.6 Sol (REQUEST_CHANGES) · GLM 5.2 (APPROVE) · Kimi K3 (APPROVE) · Claude Opus 4.8 (APPROVE)
Corrected verdict: COMMENT. The prior review said APPROVE, but GPT-5.6 Sol returned REQUEST_CHANGES with a warning-level finding (downgraded from critical via severity rubric). Per merge-readiness criteria: zero criticals, warning not acknowledged → COMMENT.
Warning (from GPT-5.6 Sol, parent-verified)
Stale files on re-export to the same directory (export.py:139)
If export_package is called with weights_dir and then re-called without it on the same out_dir, the stale weights/ directory remains while manifest.json reports has_weights: false. Consider exporting into a clean/staged directory or clearing out_dir before writing.
Non-blocking suggestions (from Claude Opus)
- [suggestion]
export.py:310—LGBMRegressor(random_state=42, verbose=-1, **HYPERPARAMETERS)will raiseTypeErrorifHYPERPARAMETERScontainsrandom_stateorverbosekeys (duplicate keyword argument). - [nit]
export.py:268— Generated_workflow()gatesapi_keyonSOURCE in ("allora", "atlas")butModelSpec.validate()only permits"binance"and"allora". The"atlas"branch is dead code.
(Reviewed by Helder's Agent — GPT-5.6 Sol + GLM 5.2 + Kimi K3 + Claude Opus 4.8)
|
Addressed the aggregated review findings. Triage:
Validated the export logic via a stdlib-only importlib harness (params merge resolves correctly, atlas fully removed) and |
helder-moreira
left a comment
helder-moreira
left a comment
There was a problem hiding this comment.
Grok 4.5 Review — REQUEST_CHANGES
Shared engineered-features extraction is correct (index/guard/eps parity, safe column names, both train/serve paths + solid offline parity tests). Blocked on export package bug: LGBMRegressor construction lets fixed kwargs collide with user hyperparameters.
(Grok 4.5 — single-model review)
… cache for containers
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Architecture diagram
sequenceDiagram
participant User
participant CLI as allora-forge-export CLI
participant Export as export_package()
participant FS as File System (output dir)
participant Model as forge_model/model.py
participant EF as apply_engineered_features()
participant NB as Training Notebook
participant Platform as Hosting Platform
Note over User,FS: EXPORT TIME
User->>CLI: allora-forge-export --config model.json --out ./pkg [--weights ./w]
CLI->>Export: Export package with ModelSpec
Export->>FS: Write forge_model/ (model.py, config.json, __init__.py)
Export->>FS: Write pyproject.toml (entry-point, builder-kit ref)
Export->>FS: Write manifest.json (model-only: model_type, supports_training, code_hash)
alt With weights
Export->>FS: Copy weights/ directory
end
Export->>FS: Write .dockerignore
Export-->>CLI: Package path
CLI-->>User: exported to ./pkg (zip & upload)
Note over NB,Platform: RUNTIME: TRAINING PATH
NB->>EF: apply_engineered_features(df, ENGINEERED_SPECS, N_INPUT_BARS)
Note over NB,EF: NEW: shared module replaces inline engineer_returns()
EF-->>NB: (df with engineered columns, added column names)
Note over Model,Platform: RUNTIME: DEPLOYMENT (SERVING / TRAIN-ON-PLATFORM)
Platform->>Model: Run model (train/predict)
Model->>Model: Read PAIR, TIMEFRAME from env
Model->>Model: Load config.json (engineered_specs, hyperparams, etc.)
Model->>EF: apply_engineered_features(live_df, ENGINEERED_SPECS, N_INPUT_BARS)
Note over Model,EF: Same function, same spec — no train/serve skew
EF-->>Model: (df with features)
opt Training on platform
Model->>Model: train_model() → LGBMRegressor.fit()
end
Model->>Model: predict() → return float
Model-->>Platform: prediction value
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
…erive metric denominator
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Ran this end-to-end locally: 157 passed / 0 failed on the branch, export exercised in both code-only and A few things worth fixing, none blocking:
|
|
Under Review. |
|
Thanks @clementupshot — addressed in 40de7f8. 1. Manifest XOR — fixed. 3. Malformed configs — fixed. 4. Upload zip — added. New 2. |
jefferythewind
left a comment
jefferythewind
left a comment
There was a problem hiding this comment.
Overall: The shared `engineered_features` module is a clean, well-tested solution to the train/serve skew problem, and the `apply_engineered_features` refactor in the walkthroughs is the right call. Two issues worth resolving before merging:
1. No user-facing documentation or example for the export path
The README has no mention of `export_package`, `ModelSpec`, `engineered_specs`, `apply_engineered_features`, or `allora-forge-export`. A user who installs this package has no way to discover the export workflow or know what a valid `model.json` looks like without reading the source or the test file.
The walkthroughs (`example_topic_69`, `example_topic_77`) are still local-deployment examples — that's correct, don't change them. But there should be at minimum a dedicated example (e.g. `notebooks/export_to_hosting.py`) walking through the full export path: build a `ModelSpec`, call `export_package`, understand what the output directory contains.
Beyond that example, the README needs to explain how the hosted worker actually behaves at runtime — none of this is currently documented anywhere:
- Default behavior (train-on-platform, `supports_training=True`): The platform trains on first run and on schedule. What does the worker serve before the first training run completes? What triggers retraining — a time schedule, a platform event? How often?
- Train-locally mode (`supports_training=False` + `--weights`): The platform never retrains; it only serves the bundled weights. What happens if weights go stale? Can weights be updated without re-exporting?
- The `--no-training` flag and `supports_training` field: Their relationship to each other and to the SDK's `TrainingRunner` needs explaining — right now the only hint is a comment in the source code.
- Environment variables the hosted worker depends on (`PAIR`, `TIMEFRAME`, `ALLORA_API_KEY`, `SUBMIT_RETURNS`, `DATA_BASE_PATH`) — none are documented for the user configuring a deployment.
The README file map and API quick reference should also surface `engineered_features.py` and `export.py`.
2. Architecture: this should live in `WorkerManager`, not as a standalone CLI
Introducing a separate `allora-forge-export` CLI creates two parallel deployment workflows with no shared interface. The `WorkerManager` already owns the "deploy to Allora" mental model for users of this kit. The right shape is one system, two deployment types — local and cloud — both managed through `WorkerManager`.
Concretely:
- `WorkerManager` should expose a method like `deploy_hosted(spec, ...)` that calls `export_package` internally and handles the upload to forge
- The monitoring story is largely already there — most worker health signals come from the chain and `WorkerMonitor` picks them up regardless of whether the worker runs locally or in a hosted container. The only methods that need explicit local/cloud branching are the container-specific ones: log tailing and heartbeat/liveness. Those should be made deployment-type-aware rather than silently only working for local workers
- This gives cloud-deployed workers the same monitoring transparency users already get locally: submission history, on-chain scores, inference values, and worker health all in one place
Keeping `export_package` and `ModelSpec` as library-level primitives is fine — they're the right level of abstraction. But the user-facing entry point should be `WorkerManager`, not a separate CLI that users have to discover independently.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Thanks @jefferythewind — clear and fair. 1. Docs + export example — agreed, doing it now. I'll add a 2. WorkerManager integration — I'd like to align on scope before writing code. I agree the primitives ( Two questions so I build the right thing:
Happy to go either way — just want to avoid building against an unsettled upload path. (The docs in #1 will describe the CLI export flow as it stands; I'll update them when the |
|
#1 done in 3ccfe2b: added |
There was a problem hiding this comment.
4 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="notebooks/export_to_hosting.py">
<violation number="1" location="notebooks/export_to_hosting.py:92">
P2: The upload step printed by this walkthrough cannot be followed after the preceding `export_package` call: this script never writes `model.json`, so `allora-forge-export` fails while reading the required `--config` path. The example could serialize the in-memory `spec` before invoking the CLI, or zip the already-created package contents directly.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:221">
P3: The example's model-type constraint allows names such as `_my_model` or `-my-model` that `export_package` rejects. Matching the documented pattern to the validator would make invalid configuration failures predictable.</violation>
<violation number="2" location="README.md:239">
P2: Train-locally packages cannot generally be moved to arbitrary pairs/timeframes because their bundled artifact is tied to the training parameters. Qualifying this sentence to code-only/train-on-platform packages would prevent users from deploying incompatible weights.</violation>
<violation number="3" location="README.md:259">
P1: A deployment that follows these documented defaults can fail before training or inference: the generated worker requires both variables, and Binance cannot parse `5min` as an interval. The table would be accurate if it marked both variables required and used Binance's `5m` spelling.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| | `PAIR` | Trading pair, e.g. `BTCUSD` | `ETHUSD` | | ||
| | `TIMEFRAME` | Bar interval, e.g. `5min`, `1h` | `5min` | |
There was a problem hiding this comment.
P1: A deployment that follows these documented defaults can fail before training or inference: the generated worker requires both variables, and Binance cannot parse 5min as an interval. The table would be accurate if it marked both variables required and used Binance's 5m spelling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 259:
<comment>A deployment that follows these documented defaults can fail before training or inference: the generated worker requires both variables, and Binance cannot parse `5min` as an interval. The table would be accurate if it marked both variables required and used Binance's `5m` spelling.</comment>
<file context>
@@ -205,6 +206,65 @@ Mainnet topics and their testnet equivalents:
+
+| Env var | Purpose | Default |
+|---------|---------|---------|
+| `PAIR` | Trading pair, e.g. `BTCUSD` | `ETHUSD` |
+| `TIMEFRAME` | Bar interval, e.g. `5min`, `1h` | `5min` |
+| `ALLORA_TOPIC_ID` | Target topic | `69` |
</file context>
| | `PAIR` | Trading pair, e.g. `BTCUSD` | `ETHUSD` | | |
| | `TIMEFRAME` | Bar interval, e.g. `5min`, `1h` | `5min` | | |
| | `PAIR` | Trading pair, e.g. `BTCUSD` | required | | |
| | `TIMEFRAME` | Bar interval, e.g. `5m`, `1h` | required | |
| # manifest.json at the extraction root) and upload to forge. The CLI can do the | ||
| # zip for you: | ||
| # | ||
| # allora-forge-export --config model.json --out build/my_lgbm_package --zip |
There was a problem hiding this comment.
P2: The upload step printed by this walkthrough cannot be followed after the preceding export_package call: this script never writes model.json, so allora-forge-export fails while reading the required --config path. The example could serialize the in-memory spec before invoking the CLI, or zip the already-created package contents directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At notebooks/export_to_hosting.py, line 92:
<comment>The upload step printed by this walkthrough cannot be followed after the preceding `export_package` call: this script never writes `model.json`, so `allora-forge-export` fails while reading the required `--config` path. The example could serialize the in-memory `spec` before invoking the CLI, or zip the already-created package contents directly.</comment>
<file context>
@@ -0,0 +1,96 @@
+# manifest.json at the extraction root) and upload to forge. The CLI can do the
+# zip for you:
+#
+# allora-forge-export --config model.json --out build/my_lgbm_package --zip
+#
+# then upload build/my_lgbm_package.zip via forge (POST /api/v1/models).
</file context>
| # then upload build/my_lgbm_package.zip to forge (POST /api/v1/models) | ||
| ``` | ||
|
|
||
| `--zip` writes the package **contents** at the archive root, so forge finds `manifest.json` at the extraction root. The generated worker is **generic over pair/timeframe** — one package can be deployed against many pairs/timeframes/topics. |
There was a problem hiding this comment.
P2: Train-locally packages cannot generally be moved to arbitrary pairs/timeframes because their bundled artifact is tied to the training parameters. Qualifying this sentence to code-only/train-on-platform packages would prevent users from deploying incompatible weights.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 239:
<comment>Train-locally packages cannot generally be moved to arbitrary pairs/timeframes because their bundled artifact is tied to the training parameters. Qualifying this sentence to code-only/train-on-platform packages would prevent users from deploying incompatible weights.</comment>
<file context>
@@ -205,6 +206,65 @@ Mainnet topics and their testnet equivalents:
+# then upload build/my_lgbm_package.zip to forge (POST /api/v1/models)
+```
+
+`--zip` writes the package **contents** at the archive root, so forge finds `manifest.json` at the extraction root. The generated worker is **generic over pair/timeframe** — one package can be deployed against many pairs/timeframes/topics.
+
+### Two deployment modes
</file context>
| `--zip` writes the package **contents** at the archive root, so forge finds `manifest.json` at the extraction root. The generated worker is **generic over pair/timeframe** — one package can be deployed against many pairs/timeframes/topics. | |
| `--zip` writes the package **contents** at the archive root, so forge finds `manifest.json` at the extraction root. The generated worker code is **generic over pair/timeframe**; code-only packages can be deployed against many pairs/timeframes/topics, while bundled-weight packages must use matching deployment parameters. |
| # Model-INTRINSIC config (baked into the package's config.json). Pair/timeframe/ | ||
| # topic are NOT here — they are chosen per deployment (see env vars below). | ||
| spec = ModelSpec( | ||
| model_type="my_lgbm", # entry-point name; [a-z0-9_-] |
There was a problem hiding this comment.
P3: The example's model-type constraint allows names such as _my_model or -my-model that export_package rejects. Matching the documented pattern to the validator would make invalid configuration failures predictable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 221:
<comment>The example's model-type constraint allows names such as `_my_model` or `-my-model` that `export_package` rejects. Matching the documented pattern to the validator would make invalid configuration failures predictable.</comment>
<file context>
@@ -205,6 +206,65 @@ Mainnet topics and their testnet equivalents:
+# Model-INTRINSIC config (baked into the package's config.json). Pair/timeframe/
+# topic are NOT here — they are chosen per deployment (see env vars below).
+spec = ModelSpec(
+ model_type="my_lgbm", # entry-point name; [a-z0-9_-]
+ engineered_specs=[{"kind": "log_return", "window_bars": 6}],
+ number_of_input_bars=24,
</file context>
| model_type="my_lgbm", # entry-point name; [a-z0-9_-] | |
| model_type="my_lgbm", # entry-point name; [a-z0-9][a-z0-9_-]* |
3 participants
What
The builder-kit side of the hosting integration — lets a user who builds a model with the builder kit deploy it:
Shared engineered-features module (
apply_engineered_features) — one implementation used by BOTH the training path (walkthroughs/notebooks) and the generated serving code, so features are byte-identical on both → no train/serve skew.export(export_package/allora-forge-exportCLI) — translates a builder-kit model into a deployable hosting package (the single-package format forge deploys):forge_model/model.py— a generatedallora_worker.BaseModelthat reproduces the builder-kit model viaAlloraMLWorkflow+ the sharedapply_engineered_features. Generic: readsPAIR/TIMEFRAMEfrom env (set per deployment), so one package deploys to many pairs/timeframes.forge_model/config.json— model-intrinsic config (features, target, hyperparameters, data source) baked in.pyproject.toml— SDK unpinned (base image provides it), builder-kit installed from a git ref, entry-point keyed onmodel_type.manifest.json— model-only (model_type,supports_training,has_weights,code_hash); no pair/timeframe/topic..dockerignore+ optional bundledweights/.Supports train-on-platform (code-only) and train-locally (bundled
weights/).Validated against the real builder-kit API: the generated model's train/predict pipeline matches the walkthroughs (
feature_close_{i}base columns →apply_engineered_features→feature_cols→dropna(subset=…+['target'])→LGBMRegressor().fit),get_live_featuresprovidesattrs["current_price"], andapi_keyis passed only for the allora/atlas source (binance rejects it).Usage
Part of the hosting-integration set: allora-sdk-py#85, worker-allora-sdk-v2#13, forge-v2#260, allora-offchain-operator#53.