Repository navigation
feat(build)!: build one prepared release in the built-in backend - #653
Conversation
karhu
changed the title
7e98536 to
bc8aea0
Compare
4234cce to
01838a1
Compare
42328ea to
478e0b0
Compare
46eda17 to
7bca41c
Compare
171c7e2 to
593d9f5
Compare
593d9f5 to
42dcabe
Compare
42dcabe to
bf51e42
Compare
karhu
changed the title
bf51e42 to
65aab49
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
packages/timenet-connectors/src/timenet_connectors/builder/backend.py— With isolation on (the default), callers ofbuild_prepared()never get theTimeFFormatErrorthe backend protocol promises for a pin mismatch. The same failure raisesTimeFFormatErrorwith isolation off. The isolated branch sends every child failure through_run_in_env(env.py:221-226), which turns it intoTimeNetBuildError. Fix: raise the same exception types on both paths. For example, the child could write its error type and message to stdout, whichbuild-prepared-releaseno longer uses, and the parent could raise that type again. At minimum, document the difference. Pre-existing pattern:run_isolateddoes the same.Why this was flagged
BuilderBackend.build_prepared(packages/timenet/src/timenet/builders.py:160-164) documentsTimeFFormatError: If the build omits or changes a pinned file, andbuild_release(coordinator/build.py:57) repeats it. The trigger is a rebuild with non-emptypinned_bywhose new files differ from the pins.TimeFWriter._check_pinned_by(writer.py:433) raisesTimeFFormatErrorin the childbuild-prepared-release. The child exits non-zero, and_run_in_env(env.py:221-226) raisesTimeNetBuildError("building org/name@ ver failed with exit code 1 ...")with only the stderr tail.TIMENET_ISOLATIONdefaults to "on" (config.py:27), so this is the normal path. With isolation off,_build_in_processlets the realTimeFFormatErrorreach the caller. So the error type depends on an environment variable. The baserun_isolatedalready collapses errors this way forbuild(). The new protocol method repeats that and so breaks its own documented contract.Verification: nit. The trigger is a
build_prepared()call with isolation on (the default) and a non-emptypinned_bywhose rebuilt files differ from the pins. On a non-zero exit_run_in_envalways raisesTimeNetBuildError(f"building {subject} failed with exit code ...")(env.py ~221-226). Anexcept TimeFFormatErrortherefore never matches on the isolated path, while it does match withTIMENET_ISOLATION=off.
65aab49 to
5e8afa7
Compare
5e8afa7 to
344d43c
Compare
ConnectorBuilder now implements build_prepared(). It builds exactly one release into a local store that already holds each parent of the release, and each payload source of such a parent. It does not plan. If the card of the connector declares another version than the release, it raises TimeFValidationError before anything runs. TIMENET_ISOLATION decides where the build runs, as for build(). With isolation on, run_isolated_build() in env.py runs the hidden command timenet-build build-release org/name@version --store PATH in the environment of the connector. It forwards --force, --keep-cache and the pins in a file. If the options have no progress callback, it also forwards --quiet. The child runs with TIMENET_ISOLATION=off. If the child fails, run_isolated_build() raises TimeNetBuildError with the tail of the stderr of the child. With isolation off, build_prepared() calls run_pipeline() with the values_backend of the connector and builds into the root of the store. It sends each download as a BytesMoved event and each finished shard as a ShardWritten event to the progress callback of the options. Without a progress callback, it installs no download sink. The build-release command calls build_prepared() of ConnectorBuilder. Unless --quiet is set, it shows the build lines, the shards and the downloads on the console. It prints nothing on stdout. A test makes sure that the command does not plan: with a parent missing from the store, it fails instead of building the parent. The bar display of the downloads moves into _download_bars(), so that the old download display and the new renderer share it. Nothing outside the tests calls build_prepared() yet. The build command, build() and run_isolated() work as before. Part of TNET-157.
ConnectorBuilder.build_prepared() now passes options.quiet to the isolated child. Before, it passed whether options.progress was None. So an SDK build, which sends no progress callback, ran the child with --quiet, and the user saw nothing of a long download. The hidden build-release command passes its own --quiet on in the options too. Another version than the card declares now raises TimeNetVersionUnavailableError, like build_release() does. It is a TimeFValidationError too, so a handler of the old error still catches it. The docstring of build_prepared() wraps near 100 columns again. Part of TNET-157.
The docstring now says what the method builds and when it needs an isolated environment. Part of TNET-157.
…plain the card-only read The hidden child command now has the same name as build_prepared(), as the review asked. The comment in build_prepared() now says why the parent reads only the card: the connector and its dependencies exist only in the environment of the child. Part of TNET-157.
Say only what a caller of the prepared build needs to know. Part of TNET-157.
The build-prepared-release child no longer sends StepStarted and StepFinished. The coordinator in the parent sends them around build_prepared(), so the child showed each step line a second time. The renderer loses its two step branches, and the command function is now build_prepared_release_command, after its command name. run_isolated() and run_isolated_build() now run the child through one private helper. run_isolated_build() takes force, keep_cache, quiet and pinned_by without defaults. Tests that repeated another test go away: the second stderr-tail test, the in-process backend build that the CLI test covers, a progress case that the child never sees, and the isolation case of the version check, which runs before isolation matters. The child-command test no longer patches the planner, because its missing-parent failure proves that the child does not plan. Part of TNET-157.
Shorter docstrings for the code that this change adds or changes. Part of TNET-157.
The build CLI drew its own download bars and status lines, and turned each BytesMoved event back into a DownloadProgress event to reuse its old reporters. timenet.cli.ui.progress_renderer() now shows every progress event: step lines, a bar for each transfer on a terminal, and throttled lines elsewhere. The build CLI uses it, and the conversion back goes. Only the old in-process build still turns the downloads of a connector into BytesMoved events. timenet_connectors.download.ProgressCallback had the name of the callback type in timenet.progress, but takes another event. It is now DownloadCallback. BREAKING CHANGE: timenet_connectors.download.ProgressCallback is now DownloadCallback. Part of TNET-157.
344d43c to
38a10b6
Compare
The Stack
This stack covers refactoring the internal logic of how releases are resolved during read, download & build, in particular also in the context of parent releases / parent datasets. On top of that it refactors the public TimeNet APIs and CLI interfaces around these actions, to streamline the behavior across them.
This PR
Adds
ConnectorBuilder.build_prepared(), so that the coordinator can build one release of a connector dataset.Details
#652 adds the build coordinator and the
build_prepared()method of the builder protocol (TNET-157). The built-in backend,ConnectorBuilder, does not implement that method yet. So the coordinator cannot build a dataset of a connector.ConnectorBuilder.build_prepared(release, store, *, force, pinned_by, options)builds exactly one release into a local store. The store already holds each parent of the release, and each payload source of such a parent.TimeNetVersionUnavailableErrorbefore anything runs. For an ID that the backend does not know, it raisesTimeNetDatasetNotFoundError.TIMENET_ISOLATIONdecides where the build runs, as forbuild().run_isolated_build()inenv.pyruns the hidden commandtimenet-build build-prepared-release org/name@version --store PATHin the environment of the connector. It forwards--force,--keep-cacheand the pins in a file. Ifoptions.quietis set, it also forwards--quiet. Otherwise the child prints its shards and downloads to the inherited stderr. If the child fails, it raisesTimeNetBuildErrorwith the tail of the stderr of the child.run_pipeline(), and the connector decides the values backend. It sends each download as aBytesMovedevent and each finished shard as aShardWrittenevent.BytesMovedandShardWrittentotimenet.progress.progress_renderer()intimenet.cli.uishows every progress event on the console. Each copy or build step and each written shard gets a line. In a terminal, each transfer gets a bar. Elsewhere, a transfer gets a line at about every 20%. Thebuild-prepared-releasecommand uses it.--quietis set, thebuild-prepared-releasecommand shows the shards and the downloads on the console. It prints nothing on stdout.timenet_connectors.download.ProgressCallbackis nowDownloadCallback, so its name differs fromtimenet.progress.ProgressCallback.Design decisions
progress_renderer(). The old path only turns eachDownloadProgressof a connector into aBytesMovedevent.Breaking changes
timenet_connectors.download.ProgressCallbackis nowDownloadCallback.Stack
Changes later in the stack:
build_prepared()yet. feat(build)!: move the build CLI and build() onto the coordinator #654 moves the build command andtimenet_connectors.build()onto it. feat(build)!: run every build through the coordinator #655 removes the oldbuild()andrun_isolated(), and renamesbuild_prepared()tobuild().progress_renderer()and removes_download_progress(). feat(build)!: run every build through the coordinator #655 makes_run_build()discard the stdout of the child.Merge message