Skip to content

Conversation

psychedelicious
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Description

fix(stats): fix fail case when previous graph is invalid

When retrieving a graph, it is parsed through pydantic. It is possible that this graph is invalid, and an error is thrown.

Handle this by deleting the failed graph from the stats if this occurs.

fix(stats): fix InvocationStatsService types

  • move docstrings to ABC
  • start_time: int -> start_time: float
  • remove class attribute assignments in StatsContext
  • add update_mem_stats() to ABC
  • add class attributes to ABC, because they are referenced in instances of the class. if they should not be on the ABC, then maybe there needs to be some restructuring

QA Instructions, Screenshots, Recordings

On main (not this PR), create a situation in which an graph is valid but will be rendered invalid on invoke. Easy way in node editor:

  • create an Integer Primitive node, set value to 3
  • create a Resize Image node and add an image to it
  • route the output of Integer Primitive to the width of Resize Image
  • Invoke - this will cause first a Validation Error (expected), and if you inspect the error in the JS console, you'll see it is a "session retrieval error"
  • Invoke again - this will also cause a Validation Error, but if you inspect the error you should see it originates in the stats module (this is the error this PR fixes)
  • Fix the graph by setting the Integer Primitive to 512
  • Invoke again - you get the same Validation Error originating from stats, even tho there are no issues

Switch to this PR, and then you should only ever get the Validation Error that that is classified as a "session retrieval error".

When retrieving a graph, it is parsed through pydantic. It is possible that this graph is invalid, and an error is thrown.

Handle this by deleting the failed graph from the stats if this occurs.
- move docstrings to ABC
- `start_time: int` -> `start_time: float`
- remove class attribute assignments in `StatsContext`
- add `update_mem_stats()` to ABC
- add class attributes to ABC, because they are referenced in instances of the class. if they should not be on the ABC, then maybe there needs to be some restructuring
@psychedelicious
Copy link
Collaborator Author

I also fixed some typing issues in the stats module.

Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

LGTM

@lstein lstein merged commit 572e6b8 into main Aug 21, 2023
@lstein lstein deleted the fix/stats/handle-exceptions branch August 21, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants