Conversation
…ation - snapshotBackends() helper clones registry under RLock - List() / All() / Default() iterate over snapshot — no longer map-iteration-order-dependent - preferredBackendOrder + preferredBackendSet codify metal > rocm > llama_cpp priority - Test: TestInference_All_Good_Alphabetical + deterministic non-preferred fallback Verified: GOWORK=off go test ./... passes Module-path rename to dappco.re/go/inference per RFC deferred — requires coordinated migration across go-ml + go-rocm consumers. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Switch registry and load-path errors over to core.E so the package follows the shared error contract from the RFC. Co-Authored-By: Virgil <virgil@lethean.io>
Align the package with the RFC's stdlib-only requirement by replacing core helpers with local stdlib equivalents. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
…on (AX-10) go-inference is a pure interface library with no shipped binary, so the Taskfile follows the AX-10 adaptation: package-integrity verification rather than binary fixture assertion. Four tasks, all executed from module root: - build: go build ./... (must compile even where no Metal backend exists) - test: go test -count=1 -race ./... (full suite with race detector) - test-discover: runs TestDiscover_Good_SingleModel — temp-dir fixture with config.json + dummy .safetensors, asserts DiscoveredModel shape - vet: go vet ./... Reuses the existing discover_test.go helper rather than introducing a new helper file outside the path allowlist. Closes tasks.lthn.sh/view.php?id=267 Signed-off-by: Snider <snider@host.uk.com>
… (AX-10 polish) Adds `default: deps: [build, test, vet]` to the existing CLI test Taskfile so bare `task -d tests/cli/<pkg>` runs the full suite per the Wave 2 convention. Closes tasks.lthn.sh/view.php?id=604 Co-Authored-By: Cladius <cladius@lthn.ai>
Closes tasks.lthn.sh/view.php?id=685 Co-authored-by: Codex <noreply@openai.com>
…AX-6)
inference.go used strconv.Quote for error-message formatting and a
sync.RWMutex on the backend registry. Swapped:
- strconv.Quote → core.Sprintf("%q", ...)
- sync.RWMutex → core.New().Lock("inference.backends").Mutex
Dropped both imports. Registry concurrency semantics unchanged; core
primitive replaces the stdlib mutex.
Closes tasks.lthn.sh/view.php?id=263
Co-authored-by: Codex <noreply@openai.com>
training.go used strconv.Quote in an error message about trainable
backends. Swapped to core.Sprintf("%q", ...) matching the pattern
applied to inference.go in 11c0d9d. Dropped the strconv import.
Closes tasks.lthn.sh/view.php?id=265
Co-authored-by: Codex <noreply@openai.com>
…cover.go (AX-6) discover.go rewrote directory scanning + config parsing to use core primitives: - encoding/json → core.JSONUnmarshalString - io/fs → core.New().Fs() medium operations - os → c.Fs().* via io.Medium - path/filepath → core.Join / core.Split / path string helpers Dropped all four imports. `go vet ./...` passes. Closes tasks.lthn.sh/view.php?id=264 Co-authored-by: Codex <noreply@openai.com>
inference_test.go + discover_test.go swapped:
- fmt.Sprintf → core.Sprintf
- errors.New → core.E("test", "...", nil)
Dropped fmt + errors imports. Test compilation passes.
Closes tasks.lthn.sh/view.php?id=605
Co-authored-by: Codex <noreply@openai.com>
Created docs/RFC.models.md with frontmatter + Synopsis + Architecture placeholder sections. CLAUDE.md index didn't reference the missing file so no index edit needed. Closes tasks.lthn.sh/view.php?id=603 Co-authored-by: Codex <noreply@openai.com>
- Bump dappco.re/go/* deps to v0.8.0-alpha.1 in go.mod (any forge.lthn.ai/core/* paths migrated to canonical dappco.re/go/* form) Co-Authored-By: Athena <athena@lthn.ai>
Removed `replace dappco.re/go/core => ../go`. Pinned dappco.re/go/core to v0.8.0-alpha.1 (now resolvable via proxy). go.sum updated. GOWORK=off go build + go mod tidy clean. Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=789
inference_test.go + discover_test.go: - errors.New → core.E - fmt.Sprintf → core.Sprintf - filepath.Join → core.JoinPath - sync + os retained with // Note: test-only annotations Race suite passes. Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=710
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (19)
Comment |
# Conflicts: # CLAUDE.md # README.md # discover.go # docs/architecture.md # docs/backends.md # docs/development.md # docs/index.md # docs/types.md # inference.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Routine dev→main sync that updates the module identity and strengthens core inference behaviors while refactoring tests and discovery utilities.
Changes:
- Renames the module path to
dappco.re/go/inferenceand updates docs/README accordingly. - Improves runtime robustness in model/trainable loading (nil model handling, consistent error wrapping) and updates generation defaults (RepeatPenalty = 1.0).
- Replaces many testify assertions with local
check*helpers and expands test coverage; adds a Taskfile for CLI test workflows.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Module path rename; Go version set; dependencies reorganized. |
go.work |
Adds workspace file pinning toolchain version. |
go.sum |
Updates dependency checksums (currently inconsistent with go.mod). |
README.md |
Updates module path/docs; adjusts badges/links (license + Go version need alignment). |
docs/index.md |
Updates import/module references to new module path. |
docs/types.md |
Updates module path reference; documents RepeatPenalty default. |
docs/development.md |
Updates module path; adjusts stated Go prerequisite (needs alignment with go.mod/go.work). |
docs/backends.md |
Updates import paths and example backend blank imports. |
docs/architecture.md |
Updates module path and default GenerateConfig behavior docs. |
docs/RFC.models.md |
Adds draft RFC placeholder. |
options.go |
Sets RepeatPenalty default to 1.0; clones stop-token slices; ignores nil options. |
options_test.go |
Migrates to check* helpers; adds tests for nil options + stop-token copy isolation. |
test_helpers_test.go |
Introduces local assertion helpers to replace testify usage. |
inference.go |
Registry mutex source change; adds nil-backend no-op; improves Default/LoadModel erroring/wrapping; nil-model checks. |
inference_test.go |
Migrates to check* helpers; adds tests for ordering/determinism and nil scenarios (currently has duplicate tests + leftover testify usage). |
training.go |
Adds nil-model guard in LoadTrainable; improves error structure. |
training_test.go |
Migrates to check* helpers; adds nil-model coverage. |
discover.go |
Rewrites Discover traversal to use core.Fs + deterministic ordering (absolute path logic currently incorrect). |
discover_test.go |
Migrates many tests to stdlib fs fixtures + check*; adds more cases (still contains leftover testify usage later). |
tests/cli/inference/Taskfile.yaml |
Adds task runner commands for build/test/vet workflows. |
Comments suppressed due to low confidence (3)
inference_test.go:199
- Duplicate test function name
TestInference_All_Good_SortedOrderappears twice in this file; the second copy also still callsassert.Equaleven though testify imports were removed. This will not compile—please delete/rename the duplicate test and convert the remaining assertions to the localcheck*helpers (or re-add/testify if that's the intent).
func TestInference_All_Good_SortedOrder(t *testing.T) {
resetBackends(t)
Register(&stubBackend{name: "beta", available: true})
Register(&stubBackend{name: "alpha", available: true})
inference_test.go:284
TestInference_Default_Good_AlphabeticalFallbackis defined twice; the later copy still usesrequire.NoError/assert.Equalwithout importing testify anymore. This causes duplicate symbol + missing identifier compile errors—remove/merge the duplicate and use thecheck*helpers consistently.
func TestInference_Default_Good_AlphabeticalFallback(t *testing.T) {
resetBackends(t)
Register(&stubBackend{name: "zeta", available: true})
Register(&stubBackend{name: "alpha", available: true})
discover_test.go:320
TestDiscover_Good_RecursiveNestedModelsis reintroduced a second time later in the file (duplicate function name). The later block also usescore.Pathand testifyrequire/asserteven though the file no longer imports testify. Please remove/rename the duplicate and migrate the assertions tocheck*(and use the same path-joining approach as the rest of the file).
func TestDiscover_Good_RecursiveNestedModels(t *testing.T) {
base := t.TempDir()
createModelDir(t, core.Path(base, "models"), map[string]any{
"model_type": "parent",
}, 1)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [](https://pkg.go.dev/dappco.re/go/inference) | ||
| [](LICENSE.md) |
There was a problem hiding this comment.
README links to LICENSE.md here, but later in the same file it references [LICENCE](LICENCE) and the repository root currently contains neither file. Please add the actual license file (and pick one consistent filename/link) so the badge and licence section aren’t broken.
| func absolutePath(dir string) string { | ||
| if core.PathIsAbs(dir) { | ||
| return cleanPath(dir) | ||
| } | ||
|
|
||
| cwd := core.Env("DIR_CWD") | ||
| if cwd == "" { | ||
| return cleanPath(dir) | ||
| } |
There was a problem hiding this comment.
absolutePath can return a relative path when dir is relative and DIR_CWD is unset (it falls back to cleanPath(dir)). That violates the DiscoveredModel.Path contract (“Absolute path”) and will break callers/tests expecting filepath.IsAbs(Path) to be true. Prefer computing an actual absolute path (e.g., via the real working directory + join/clean, or filepath.Abs).
| **Module**: `dappco.re/go/inference` | ||
| **Licence**: EUPL-1.2 | ||
| **Language**: Go 1.26 | ||
| **Language**: Go 1.25 |
There was a problem hiding this comment.
The README is inconsistent about the Go version: the badge shows Go 1.26, but the “Language” line says Go 1.25, while go.mod/go.work specify Go 1.26.0. Please make these consistent to avoid confusing consumers and contributors.
| **Language**: Go 1.25 | |
| **Language**: Go 1.26 |
| ## Prerequisites | ||
|
|
||
| - Go 1.26 or later (uses `iter.Seq`, `maps`, `slices`) | ||
| - Go 1.25 or later (uses `iter.Seq` from Go 1.23 and range-over-function from 1.22) |
There was a problem hiding this comment.
Docs say “Go 1.25 or later”, but this PR sets go.mod/go.work to Go 1.26.0. Please align the documented prerequisite with the actual go directives (or lower the directives if 1.25 is sufficient).
| - Go 1.25 or later (uses `iter.Seq` from Go 1.23 and range-over-function from 1.22) | |
| - Go 1.26 or later (uses `iter.Seq` from Go 1.23 and range-over-function from 1.22) |
| require github.com/stretchr/testify v1.11.1 | ||
|
|
||
| require dappco.re/go/core v0.8.0-alpha.1 |
There was a problem hiding this comment.
go.mod still requires github.com/stretchr/testify, but the updated tests in this PR have largely switched to local check* helpers and no Go files reference testify anymore. If the remaining testify usage is accidental, consider dropping the dependency (and running go mod tidy) to keep the module truly stdlib-only.
| "slices" | ||
| "testing" | ||
|
|
||
| "dappco.re/go/core" |
There was a problem hiding this comment.
This file drops testify imports, but there are still require.* / assert.* calls later in the file (e.g. the second TestDiscover_Good_RecursiveNestedModels block near the end). As-is the test package won’t compile—either remove/convert those remaining testify assertions to check*, or keep the testify imports until the migration is complete.
| "dappco.re/go/core" | |
| "dappco.re/go/core" | |
| "github.com/stretchr/testify/assert" | |
| "github.com/stretchr/testify/require" |
| func pathSeparator() string { | ||
| if separator := core.Env("DS"); separator != "" { | ||
| return separator | ||
| } | ||
| return "/" | ||
| } |
There was a problem hiding this comment.
pathSeparator() defaults to "/" unless the non-standard DS env var is set. On Windows this will produce incorrect paths for fsys.List / fsys.Read and can prevent discovery from working at all. Consider using the OS path separator (os.PathSeparator/filepath.Separator) rather than an env override, or at least default to the OS separator when DS is not set.
Routine dev→main PR opened by Hephaestus PR-cadence task.
This PR exists to:
ahead_by: 32 commit(s) (per gh api compare)If CodeRabbit clears with no blocking comments, this PR is eligible for
gh pr merge --merge(real merge commit, no force-push). Conflicts and review feedback should be addressed on the dev branch before merge.Co-authored-by: Hephaestus hephaestus@cladius