Review — ITL-564: kickoff + Leg 0 (foundations)

src/orcapod/channels.py

@@ -7,7 +7,7 @@
 from __future__ import annotations
 
 import asyncio
-from collections.abc import AsyncIterator
+from collections.abc import AsyncIterator, Sequence
 from dataclasses import dataclass, field
 from typing import Generic, Protocol, TypeVar, runtime_checkable
 
@@ -41,7 +41,6 @@ class ChannelClosed(Exception):
 # Protocol types
 # ---------------------------------------------------------------------------
 
-
 @runtime_checkable
 class ReadableChannel(Protocol[T_co]):
     """Consumer side of a channel."""
@@ -54,8 +53,8 @@ def __aiter__(self) -> AsyncIterator[T_co]: ...
 
     async def __anext__(self) -> T_co: ...
 
-    async def collect(self) -> list[T_co]:
-        """Drain all remaining items into a list."""
+    async def collect(self) -> Sequence[T_co]:
+        """Drain all remaining items into a sequence."""
         ...
 
 

src/orcapod/config.py

@@ -1,4 +1,18 @@
 # config.py
+#!? SECOND config system: OrcapodConfig (hashing/display/datetime, TOML-loaded, global) is entirely
+#!? separate from the execution configs in types.py (PipelineConfig/PodConfig/NodeConfig). The two
+#!? even use DIFFERENT merge semantics — here "non-DEFAULT value in other overrides"; NodeConfig uses
+#!? "non-NONE in other overrides". A reader must know which convention applies where. Consider
+#!? documenting the split (library/global vs per-execution) and unifying the merge philosophy.
+#! There are legitimately two types of "config" although their interfaces should be as intuitively similar as possible
+#! Namely one set of configs are what would be passed into configure target objects such as Pod, Node, etc. You'd expect
+#! to create these configs and then pass them in. On the other than, there is the second category of config(s) which is
+#! the configuration of Orcapod itself, largely globally. A lot of top level system in Orcapod can optionally take in an
+#! instance of OrcapodConfig to override the default config set globally but canonical usage would be to configure
+#! the entire library globally. We should remove unnecessary duplication/divergence such as disagreeing implementation of
+#! merge but otherwise, we should make it very clear when something is about configuring Orcapod itself, whereas other is
+#! about configuring specific objects/system within Orcapod library. We should consider actually have object-specific config
+#! to reside *together* with the object it is meant to configure. This needs design spike for evaluation.
 from __future__ import annotations
 
 import logging
@@ -10,7 +24,7 @@
 logger = logging.getLogger(__name__)
 
 
-@dataclass(frozen=True)
+@dataclass(frozen=True, slots=True)
 class _SectionConfig:
     """Base class for frozen leaf config section dataclasses.
 
@@ -22,6 +36,16 @@ class _SectionConfig:
     so that ``type(self)()`` produces a valid defaults instance.
     """
 
+    #!? DEAD + subtly broken. `_SectionConfig.merge` is only reached via `OrcapodConfig.merge`,
+    #!? which itself has NO external callers (verified) — `load_config` deliberately avoids it.
+    #!? WHY it's avoided: "non-default overrides" cannot express "reset a field back to its default"
+    #!? (a default in `other` reads as "unset"), which is exactly what `load_config` needs for a
+    #!? higher-precedence file to win. So merge() cannot do the one job the module actually requires.
+    #!? Decide: remove the merge() chain, or redesign it with an explicit "unset" sentinel and route
+    #!? load_config through it (killing the duplication below).
+    #! This provides confusing diverging implementation to merge logic found in some config object found in types.py
+    #! The implementations and inheritance hierarchy (if there is to be any) should be clearned up and
+    #! fully documented to avoid confusion
     def merge(self, other: Self) -> Self:
         """Return a new instance with non-default fields from ``other`` applied.
 
@@ -47,7 +71,7 @@ def merge(self, other: Self) -> Self:
         return replace(self, **updates)
 
 
-@dataclass(frozen=True)
+@dataclass(frozen=True, slots=True)
 class DatetimeConfig(_SectionConfig):
     """Datetime handling policy settings.
 
@@ -69,7 +93,7 @@ class DatetimeConfig(_SectionConfig):
     timezone_policy: Literal["strict", "coerce_utc"] = "strict"
 
 
-@dataclass(frozen=True)
+@dataclass(frozen=True, slots=True)
 class HashingConfig(_SectionConfig):
     """Hash length settings for system-tag column names, schema hashes, and path scoping.
 
@@ -122,6 +146,8 @@ def with_updates(self, **kwargs: Any) -> Self:
         """
         return replace(self, **kwargs)
 
+    #! yet another implementation of merge that should be avoided
+    #! as mentioned above, we should consolidate config merging logic
     def merge(self, other: "OrcapodConfig") -> "OrcapodConfig":
         """Merge with another ``OrcapodConfig``; other takes precedence for non-default values.
 
@@ -169,6 +195,14 @@ def from_dict(
             ``OrcapodConfig`` populated from ``data``; missing sections and fields
             fall back to built-in defaults.
         """
+        #! Indeed this is non-sense duplication -- there ought to be targetted issue(s) to address/refactor
+        #! duplication
+        #!? MASSIVE duplication: the unknown-section-warn + unknown-field-warn + filter block is
+        #!? copy-pasted 3× here (hashing/display/datetime) AND re-implemented again in load_config()
+        #!? below (~150 lines total). Extract one helper `_parse_section(name, raw, cls, path_str)`
+        #!? and have BOTH from_dict and load_config call it. Also: from_dict checks isinstance(...,
+        #!? Mapping) while load_config checks isinstance(..., dict) — pick one. The known-sections set
+        #!? {"hashing","display","datetime"} is hardcoded in 3 places; derive from the dataclass fields.
         known_sections = {"hashing", "display", "datetime"}
         path_str = f" in {source_path}" if source_path is not None else ""
 
@@ -261,6 +295,12 @@ def load_config(
     """
     import tomllib
 
+    #! indeed this should make use of existing methods rather than providing bare implementation again
+    #! furthermore, config should optionally support getting values set through env vars (low priority)
+    #!? Reimplements from_dict()'s per-section parsing instead of reusing it. The "apply only keys
+    #!? explicitly present so a higher-precedence file can reset to default" trick (below) is the real
+    #!? reason merge() is bypassed — see the note on _SectionConfig.merge. If a shared _parse_section
+    #!? helper returned the explicit-keys dict, load_config could layer files via replace() cleanly.
     _user_path = (
         Path(user_config_path)
         if user_config_path is not None

src/orcapod/errors.py

@@ -1,3 +1,14 @@
+#!? No common base class. The 14 errors here inherit from a mix of Exception(7)/ValueError(3)/
+#!? RuntimeError(3)/LookupError(1) — a caller cannot `except OrcapodError`. Consider a single
+#!? `OrcapodError(Exception)` root (builtin-semantic subclasses can still multiply-inherit if wanted).
+#!? Also: error classes are NOT all here — ContextValidationError/ContextResolutionError live in
+#!? contexts/core.py, SemanticHashingError in hashing/visitors.py. Decide whether errors.py is the
+#!? single registry (re-export at least) or accept scatter. Not re-exported from top-level __init__.
+#!? MISSING (Area 3 / ITL-557): no execution-failure taxonomy at all — no PodExecutionError / OOM /
+#!? Transient / Retryable types. This is the groundwork the retry+memory-scaling work needs, and the
+#!? reason InvocationStatus (hooks.py) has only a single undifferentiated ERROR bucket.
+#! All errors defined should ultimately inherit from OrcapodError(Exception) and also all exception/error
+#! classes must be present/collected in this module. We do want to add execution-failure taxonomy
 class InputValidationError(Exception):
     """
     Exception raised when the inputs are not valid.
@@ -64,6 +75,8 @@ class SourceSpecMismatchError(ValueError):
 
     The class name ``SourceSpecMismatchError`` is preserved for compatibility
     with any code that catches it by name.
+    #!? "preserved for compatibility" — this project is pre-v0.1 greenfield (CLAUDE.md: no
+    #!? back-compat shims). If nothing external depends on the old name, drop this note / rename freely.
 
     Contains the slot name and a description of the incompatible field(s).
     Raised at ``bind()`` time — schema mismatches are rejected before execution.
@@ -148,7 +161,10 @@ def __init__(self, empty_data: object) -> None:
         super().__init__(
             "content_hash() called on EmptyData with no cached_content_hash. "
             "The pipeline DB row that produced this token is missing the stored "
-            "hash column (OUTPUT_DATA_HASH_COL or INPUT_DATA_HASH_COL). "
+            "hash column (OUTPUT_DATA_HASH_COL). "
+            #!? Misleading: flow-through keys ONLY off OUTPUT_DATA_HASH_COL (function_node.py:2009);
+            #!? INPUT_DATA_HASH_COL is written but never read for flow-through (dead — Area 4 finding).
+            #!? Drop the INPUT reference from this message.
             "Flow-through is unavailable for this row."
         )
 

src/orcapod/system_constants.py

@@ -1,32 +1,58 @@
+##! Add detailed docstring to summarize key categories of constants and how they are used/what do they control
+
+
 # Constants used for source info keys
+#! Must add appropriate comment to clarify the use of each & every constant
 SYSTEM_COLUMN_PREFIX = "__"
 DATAGRAM_PREFIX = "_"
 SOURCE_INFO_PREFIX = "source_"
-POD_ID_PREFIX = "pod_id_"
-PF_VARIATION_PREFIX = "pf_var_"
-PF_EXECUTION_PREFIX = "pf_exec_"
+
+#! Indeed it's not clear what this was meant to be used for -- I'm imagining things that used to be part of the fucntion
+#! data is now captured by the data function's own e.g. variation datagram
+POD_ID_PREFIX = "pod_id_"  #!? dead: 0 uses outside this file (its property below is also unused). Wire or remove.
+PF_VARIATION_PREFIX = "pf_var_" #! Needs comment to clarify what this is -- applies to everything
+PF_EXECUTION_PREFIX = "pf_exec_" #! Very likely PF = packet_function which is actually an old naming for data function
 DATA_CONTEXT_KEY = "context_key"
-INPUT_DATA_HASH_COL = "input_data_hash"
+INPUT_DATA_HASH_COL = "input_data_hash" #! There is constant naming inconsistency -- generally remove "col" and have hash named after it's content
 OUTPUT_DATA_HASH_COL = "output_data_hash"
-DATA_RECORD_ID = "data_id"
 NODE_CONTENT_HASH_COL = "node_content_hash"
+DATA_RECORD_ID = "data_id"  #! Add more explanation as to what this is about
 SYSTEM_TAG_PREFIX_NAME = "tag"
 SYSTEM_TAG_SOURCE_ID_FIELD = "source_id"
 SYSTEM_TAG_RECORD_ID_FIELD = "record_id"
-POD_VERSION = "pod_version"
-EXECUTION_ENGINE = "execution_engine"
-POD_TIMESTAMP = "pod_ts"
+
+#! This is indeed very likely no longer relevant -- corresponding but more refined versioning measure of pod occurs now through versioning of data function
+#! concretely, noting version info of the OSS code, running config, etc
+
+#! This variable is likely no longer relevant -- verify it's not used anywhere and just delete this constant
+POD_VERSION = "pod_version"  #!? dead: intended pod-version system column never wired. Version lives only in the table path + pf_var_* observational cols (Area 2). Decide: wire as a real column or remove.
+
+EXECUTION_ENGINE = "execution_engine"  #!? dead: intended executor-identity column never wired. Executor info only in pf_exec_* observational cols; never in pipeline/tag records (Area 2, X2).
+POD_TIMESTAMP = "pod_ts" #! If this is still used, it's awkward as to why POD_TIMESTAMP is left alone
+
+#! Must add explanation as to what is the semantic differentiation between the field and block separator with concrete examples
 FIELD_SEPARATOR = ":"
 BLOCK_SEPARATOR = "::"
-ENV_INFO = "env_info"
-IS_EPHEMERAL_COL = "is_ephemeral"
+
+#! If completed outdated/not used, we should consider fully dropping this
+ENV_INFO = "env_info"  #!? dead: 0 uses. Env/provenance stamping never implemented (overlaps PLT-1950).
+IS_EPHEMERAL_COL = "is_ephemeral" #! Give short description
+
+#! The following are highly related and should be given its own subsection
 PIPELINE_DB_SCHEMA_VERSION = "pdb_v1"
 RESULT_DB_SCHEMA_VERSION = "rdb_v1"
 TRACKING_DB_SCHEMA_VERSION = "tdb_v1"
 
 
+#! Every constant should be further investigated to ensure that the use of particular prefix
+#! makes sense and it actually happens through the use of proper prefix as provided by the class's property, rather
+#! than skipping and obtaining information directly from the constants as found in this file
+#! E.g., use self.BLOCK_SEPARATOR instead of the bare module-level constant/variable BLOCK_SEPARATOR
 class SystemConstant:
     def __init__(self, global_prefix: str = ""):
+        #!? global_prefix is never set non-empty anywhere — only `constants = SystemConstant()` exists.
+        #!? Inert namespacing hook: all the f-string prefixing below is dead flexibility. Keep only if a
+        #!? multi-tenant/namespaced column scheme is actually planned for v0.2; otherwise simplify.
         self._global_prefix = global_prefix
 
     @property
@@ -114,4 +140,7 @@ def IS_EPHEMERAL_COL(self) -> str:
         return f"{self._global_prefix}{SYSTEM_COLUMN_PREFIX}{IS_EPHEMERAL_COL}"
 
 
+# create a singleton instance for use everywhere
+#! We are going to stick to singleton design IN CASE there will ever be a future where we want to model closely
+#! that cannot be covered by the use of fetch API per se
 constants = SystemConstant()