feat: add atomic backend storage and SDK for experiment tagging#1076
Open
tonypzy wants to merge 4 commits intofossasia:masterfrom
Open
feat: add atomic backend storage and SDK for experiment tagging#1076tonypzy wants to merge 4 commits intofossasia:masterfrom
tonypzy wants to merge 4 commits intofossasia:masterfrom
Conversation
Contributor
Reviewer's GuideImplements atomic, concurrent-safe environment storage and a lightweight tag index to support experiment tagging, adds server endpoints and websocket broadcasts for tag updates/sync, and exposes new Python client APIs for setting and retrieving tags while maintaining backward compatibility with existing env files. Sequence diagram for experiment tag set and broadcastsequenceDiagram
actor User
participant VisdomClient as VisdomClient_Python_SDK
participant HTTP as HTTP_TagsEndpoint
participant TagsHandler as TagsHandler
participant App as VisdomServerApp
participant State as EnvState
participant FS as FileSystem
participant WSSubs as WebSocketSubscribers
User->>VisdomClient: set_tags(tags, env, append)
VisdomClient->>HTTP: POST /tags { eid, tags, append }
HTTP->>TagsHandler: route_request
TagsHandler->>TagsHandler: extract_eid(args)
TagsHandler->>State: ensure state[eid] exists with jsons, reload, tags
alt append is True
TagsHandler->>State: merge tags into state[eid].tags
else append is False
TagsHandler->>State: replace state[eid].tags
end
TagsHandler->>App: update App.tags[eid]
App->>FS: save_tag_index() using atomic_save(tags_index.json)
TagsHandler->>WSSubs: broadcast_tags(eid, state[eid].tags)
WSSubs-->>User: websocket message { command: tags_update, data: { eid, tags } }
TagsHandler->>FS: serialize_env(state, [eid])
FS->>FS: acquire WRITE_LOCKS[eid]
FS->>FS: atomic_save(eid.json, json_dump(state[eid]))
FS-->>FS: release WRITE_LOCKS[eid]
TagsHandler-->>VisdomClient: HTTP response [tags]
VisdomClient-->>User: return tags
Sequence diagram for tag synchronization on websocket connectionsequenceDiagram
participant Client as WebSocketClient
participant SocketHandler as SocketHandler
participant App as VisdomServerApp
Client->>SocketHandler: open()
SocketHandler->>SocketHandler: register subscription
SocketHandler->>SocketHandler: broadcast_layouts([self])
SocketHandler->>SocketHandler: broadcast_envs([self])
SocketHandler->>SocketHandler: sync_tags([self])
SocketHandler->>App: read App.tags (tags_index.json already loaded)
SocketHandler-->>Client: websocket message { command: tags_sync, data: tags_map }
Updated class diagram for tagging-related server and client structuresclassDiagram
class Visdom {
+string server
+int port
+string base_url
+string env
+set_tags(tags, env, append)
+get_tags(env)
+_send(msg, endpoint, create)
}
class VisdomServerApp {
+string env_path
+dict state
+dict subs
+bool login_enabled
+dict tags
+load_state()
+load_tag_index()
+save_tag_index()
}
class TagsHandler {
+dict state
+string env_path
+dict subs
+bool login_enabled
+VisdomServerApp app
+initialize(app)
+post()
}
class LazyEnvData {
-string _env_path_file
-dict _raw_dict
+lazy_load_data()
+__getitem__(key)
+__iter__()
+__len__()
}
class ServerUtils {
+defaultdict WRITE_LOCKS
+atomic_save(path, data)
+serialize_env(state, eids, env_path)
+broadcast_envs(handler, target_subs)
+broadcast_tags(handler, eid, tags, target_subs)
+sync_tags(handler, target_subs)
}
class SocketHandler {
+dict subs
+VisdomServerApp application
+open()
+broadcast_layouts(target_subs)
}
VisdomServerApp "1" o-- "many" TagsHandler : uses
VisdomServerApp "1" o-- "many" SocketHandler : websocket_handlers
VisdomServerApp "1" o-- "many" LazyEnvData : lazy_env_files
VisdomServerApp "1" o-- "1" ServerUtils : file_and_broadcast_helpers
TagsHandler --> VisdomServerApp : app
TagsHandler --> ServerUtils : serialize_env
TagsHandler --> ServerUtils : broadcast_tags
LazyEnvData --> ServerUtils : lazy_load_data
SocketHandler --> ServerUtils : broadcast_envs
SocketHandler --> ServerUtils : sync_tags
Visdom --> VisdomServerApp : talks_via_HTTP
Visdom --> TagsHandler : /tags endpoint
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Concurrent updates to
self.tagsandtags_index.jsoninTagsHandler.postandApp.save_tag_indexare not guarded by any lock, so you may want to add a small global/index-level lock to avoid races or partial overwrites when multiple environments update tags at the same time. - The
TagsHandler.postendpoint multiplexes read and write semantics based on the presence of thetagskey, which makes the API harder to reason about; consider adding an explicit GET handler (or a separate read endpoint) instead of overloading POST. - In
Visdom.get_tags, the constructedurlis not used and the SDK still goes through_send(POST), which is slightly misleading—either use the direct HTTP call with that URL or remove the unused variable/comment and make the POST-based behavior explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Concurrent updates to `self.tags` and `tags_index.json` in `TagsHandler.post` and `App.save_tag_index` are not guarded by any lock, so you may want to add a small global/index-level lock to avoid races or partial overwrites when multiple environments update tags at the same time.
- The `TagsHandler.post` endpoint multiplexes read and write semantics based on the presence of the `tags` key, which makes the API harder to reason about; consider adding an explicit GET handler (or a separate read endpoint) instead of overloading POST.
- In `Visdom.get_tags`, the constructed `url` is not used and the SDK still goes through `_send` (POST), which is slightly misleading—either use the direct HTTP call with that URL or remove the unused variable/comment and make the POST-based behavior explicit.
## Individual Comments
### Comment 1
<location path="py/visdom/server/handlers/web_handlers.py" line_range="705-710" />
<code_context>
+ if eid not in self.state:
+ self.state[eid] = {"jsons": {}, "reload": {}, "tags": []}
+
+ if append:
+ current_tags = set(self.state[eid].get("tags", []))
+ current_tags.update(tags)
+ self.state[eid]["tags"] = list(current_tags)
+ else:
+ self.state[eid]["tags"] = list(set(tags))
+
+ # Update global index
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Deduplicating tags via set() changes tag ordering, which may be surprising for clients that rely on order.
Both the append and replace paths use `set(...)` for deduplication, which also reorders tags. If clients rely on user-specified tag order (e.g., for sorting or UI), this is a behavior change. To dedupe while preserving order, you could use something like:
```python
seen = set()
self.state[eid]["tags"] = [t for t in tags if not (t in seen or seen.add(t))]
```
</issue_to_address>
### Comment 2
<location path="py/visdom/__init__.py" line_range="844-855" />
<code_context>
+ env = self.env
+
+ try:
+ url = "{0}:{1}{2}/tags".format(
+ self.server, self.port, self.base_url
+ )
+ # We use a custom GET or POST here. Our handler currently only has POST.
+ # But let's check if we should support GET for simpler retrieval.
</code_context>
<issue_to_address>
**suggestion:** The constructed `url` in get_tags is unused, which can be confusing and suggests dead code.
Since `url` is never used and `_send` is called with `endpoint="tags"` instead, this looks like dead code and the comment about GET vs POST is misleading. Please either remove `url` and the comment, or refactor to actually use this URL in a direct HTTP call so the method’s intent is explicit.
```suggestion
try:
return self._send(
msg={"eid": env},
endpoint="tags",
create=False,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
1 participant
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.
Description
This PR implements the core backend infrastructure for the Experiment Tagging feature (Phase 1). It focuses on providing a reliable, concurrent-safe, and performant way to store and manage environment metadata without breaking existing workflows.
Key Highlights
write-to-temp + renamepattern usingos.replaceto prevent file corruption during server crashes.WRITE_LOCKS) to ensure data integrity during simultaneous updates.tags_index.jsonto store metadata, preserving Visdom's lazy-loading mechanism for large environment files.vis.set_tags()andvis.get_tags()to the Python client for programmatic experiment organization.LazyEnvDatato handle legacy files without thetagsfield gracefully.Testing & Verification
I have verified the implementation with the following tests:
atomic_saveandLazyEnvDatalogic.Related Issue
Resolves #1075
Next Steps
EnvControls.js.Summary by Sourcery
Add backend support for experiment tags with atomic environment persistence and websocket synchronization.
New Features:
set_tagsandget_tagsfor programmatic management of environment tags.tags_index.jsonfile to store and quickly load tags metadata across environments.Enhancements:
mainenvironment with an empty tags field during server startup.