Skip to content

Update history api to array #8890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Update history api to array #8890

wants to merge 13 commits into from

Conversation

ric-yu
Copy link

@ric-yu ric-yu commented Jul 12, 2025 •
edited
Loading

Context

This PR refactors the /history_v2 API endpoints to improve scalability and usability by compacting responses, preparing for pagination support, and enhancing data clarity.

This PR

  • Introduces a /history_v2 endpoint that:
    • provides historys in an array, so the order of histories is explicit
    • removes unused fields to reduce the size of the response
    • clarifies the prompt response by returning it as a JSON object
    • is returned as a JSON object to allow room for more properties (e.g. pagination support)
  • Introduces a /history_v2/:prompt_id that:
    • clarifies the prompt response by returning it as a JSON object
    • allows us to fetch individual workflows for prompts (they are no longer included in the /history_v2 response)

Screen recording

@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 04364f3 to 8e2f14f Compare July 15, 2025 01:04
christian-byrne
christian-byrne reviewed Jul 18, 2025
View reviewed changes
christian-byrne
christian-byrne reviewed Jul 18, 2025
View reviewed changes
Copy link
Collaborator

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

guill
guill previously approved these changes Jul 19, 2025
View reviewed changes
Copy link
Collaborator

@guill guill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@guill
Copy link
Collaborator

guill commented Jul 19, 2025 •
edited
Loading

(Other than the failingruff check due to whitespace issues.)

richyu and others added 13 commits July 20, 2025 14:11
@richyu @claude @ric-yu
Add ordered history API endpoint with deprecation warnings
80ea3d8 
- Add new /history_v2 endpoint that returns history in array format
- Revert /history endpoint to original object format for backward compatibility
- Add deprecation warnings to legacy /history endpoints
- Add get_ordered_history() method to PromptQueue class
- Add comprehensive tests for both history endpoints

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ric-yu
make execution.py more pythonic
4a21727
@ric-yu
remove prompt[2] and prompt[4] from history_v2 response
565b537
@ric-yu
remove unused "completed" and cached node info from history_v2 response
a2b8688
@ric-yu
remove workflow json from history_v2 response - client will need to u…
fdc2a53 
…se /history_v2/:prompt_id
@ric-yu
return prompt as dict rather than array
36e7363
@ric-yu
deep copy history
0609136
@ric-yu
update /history_v2/:prompt_id to also have prompts in dict fmt
c9a5ecd
@ric-yu
document the api output - it's not very clear
e79f925
@ric-yu
clean up comments
b8963ef
@ric-yu
create new dict instead of deepcopy
72ec6ce
@ric-yu
remove extra comments
b64df11
@ric-yu
fix lint
94684db
@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 2e13882 to 94684db Compare July 20, 2025 21:11
@ric-yu
Copy link
Author

ric-yu commented Jul 20, 2025

Rebasing off latest master

guill
guill approved these changes Jul 21, 2025
View reviewed changes
Copy link
Collaborator

@guill guill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

In the future though, once you've received some reviews, please merge master in rather than rebasing. Rebasing (and the required force push associated with it) make it difficult to see what's changed or to reference old comments on code.

comfyanonymous
comfyanonymous reviewed Jul 25, 2025
View reviewed changes
server.py
prompt_id = request.match_info.get("prompt_id", None)
return web.json_response(self.prompt_queue.get_history(prompt_id=prompt_id))

@routes.get("/history_v2")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a better name like ordered_history

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the decision was to use this name because we're moving from the old endpoint later on cc: @webfiltered @guill to confirm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the intention was to deprecate the /history endpoint eventually in favor of this one (so we don't have to maintain both).

I don't have a particularly strong opinion on the name though. Any of:

  • history_v2
  • history_info
  • workflow_history
  • user_history
  • prompt_history
  • ordered_history
    seem reasonable to me.

comfyanonymous
comfyanonymous reviewed Jul 25, 2025
View reviewed changes
server.py

return web.json_response(self.prompt_queue.get_ordered_history(max_items=max_items, offset=offset))

@routes.get("/history_v2/{prompt_id}")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we agreed we'd update this endpoint as well cc: @guill @webfiltered

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was specifically my recommendation to avoid a case where people need to remember to use one endpoint for the list and one for individual items (which goes against normal REST principles). It's much easier if we're just able to say "move to this new endpoint as we'll be deprecating /history eventually".

I don't have a strong opinion on the specific name we use, but do strongly feel we should keep this route to preserve consistency.

@comfyanonymous
Copy link
Owner

b850d9a

Use this and remove everything from execution.py

@ric-yu
Copy link
Author

ric-yu commented Aug 3, 2025

b850d9a

Use this and remove everything from execution.py

Sounds good, I'll make this change

@ric-yu ric-yu marked this pull request as draft August 3, 2025 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guill guill guill approved these changes

@christian-byrne christian-byrne christian-byrne left review comments

@comfyanonymous comfyanonymous comfyanonymous left review comments

@yoland68 yoland68 Awaiting requested review from yoland68

@robinjhuang robinjhuang Awaiting requested review from robinjhuang

@webfiltered webfiltered Awaiting requested review from webfiltered

@pythongosssss pythongosssss Awaiting requested review from pythongosssss

@ltdrdata ltdrdata Awaiting requested review from ltdrdata

@Kosinkadink Kosinkadink Awaiting requested review from Kosinkadink Kosinkadink is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ric-yu @guill @comfyanonymous @christian-byrne @richyu