Add public OneDep session metadata API#8
Draft
KingAlejandro wants to merge 2 commits into
Draft
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a public API for retrieving local OneDep session metadata (and full session details) so DSP adapters can list/resume sessions without parsing session.json directly.
Changes:
- Introduces
SessionSummaryand new facade functions:list_session_metadata(),get_session(), andget_session_metadata(). - Adds a public
base_dirparameter todeposit_init()/deposit_resume()to allow adapters to choose the session storage location. - Extends unit tests to cover the new entry points and
base_dirusage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit/test_deposition_facade.py | Adds unit tests for base_dir resume and the new session metadata APIs. |
| src/onedep_lib/session/models.py | Adds SessionSummary dataclass used for adapter-facing metadata. |
| src/onedep_lib/dsp.py | Implements session summary helpers and new retrieval functions; adds base_dir support to init/resume. |
| src/onedep_lib/init.py | Re-exports the new public session metadata APIs and SessionSummary. |
Comments suppressed due to low confidence (2)
src/onedep_lib/dsp.py:124
- Adding
base_dirin the middle of the parameter list changes positional argument ordering for_base_dir,_api_client, and_check_runner, which can break downstream callers that passed those arguments positionally. To keep backward compatibility, keep the existing positional parameters unchanged and makebase_dira keyword-only argument at the end.
def deposit_init(
email: str,
users: list[str],
country: Country,
experiment_type: ExperimentType | None = None,
em_subtype: EMSubType | None = None,
coordinates: bool | None = None,
config: DepositConfig | None = None,
base_dir: Path | None = None,
_base_dir: Path | None = None,
_api_client: ApiClient | None = None,
_check_runner: CheckRunnerProtocol | None = None,
) -> Deposition:
src/onedep_lib/dsp.py:175
- Same positional-argument compatibility issue as
deposit_init: insertingbase_dirbefore_base_dirshifts the positional indices of the testing overrides and can break existing callers. Makingbase_dirkeyword-only at the end preserves the previous positional signature.
def deposit_resume(
session_id: str,
config: DepositConfig | None = None,
base_dir: Path | None = None,
_base_dir: Path | None = None,
_api_client: ApiClient | None = None,
_check_runner: CheckRunnerProtocol | None = None,
) -> Deposition:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
193
to
196
| config = config or DepositConfig.load() | ||
| base_dir = _base_dir or config.session_dir | ||
| store: SessionStore = JsonSessionStore(session_id, base_dir=base_dir) | ||
| session_base_dir = _session_base_dir(config, base_dir=base_dir, _base_dir=_base_dir) | ||
| store: SessionStore = JsonSessionStore(session_id, base_dir=session_base_dir) | ||
| store.get_session() # raises KeyError if not found |
Collaborator
There was a problem hiding this comment.
Hi Alex, try the following after line 194 (session_base_dir = ...)
if session_id not in (s.session_id for s, _ in list_sessions(base_dir=session_base_dir)):
raise KeyError(f"Session with id {session_id} not found.")
a8b1e6d to
7c051ca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a small proposal for how DSP adapters could interact with OneDep Lib session storage without needing to know the details of the local
session.jsonlayout.sequenceDiagram participant Adapter as DSP adapter participant OneDep as OneDep Lib participant Store as Local session store Adapter->>OneDep: list_session_metadata(base_dir) OneDep->>Store: read local sessions Store-->>OneDep: session summaries OneDep-->>Adapter: SessionSummary[] Adapter->>Adapter: user selects existing session or new session alt Resume existing session Adapter->>OneDep: deposit_resume(session_id, base_dir) else Create new session Adapter->>OneDep: deposit_init(..., base_dir) end OneDep-->>Adapter: DepositionAt first I started parsing the json session files, but I don't think that should be owned by the adapters necessarily, with these changes