Skip to content

Conversation

@lstein
Copy link
Collaborator

@lstein lstein commented Mar 15, 2023

This PR fixes #2951 and restores the step_callback argument in the refactored generate() method. Note that this issue states that "something is still wrong because steps and step are zero." However, I think this is confusion over the call signature of the callback, which since the diffusers merge has been callback(state:PipelineIntermediateState)

This is the test script that I used to determine that step is being passed correctly:


from pathlib import Path
from invokeai.backend import ModelManager
from invokeai.backend.globals import global_config_dir
from invokeai.backend.generator import Txt2Img
from invokeai.backend.stable_diffusion import PipelineIntermediateState

def my_callback(state:PipelineIntermediateState, total_steps:int):
    print(f'callback(step={state.step}/{total_steps})')

def main():
    manager = ModelManager(Path(global_config_dir()) / "models.yaml")
    model = manager.get_model('stable-diffusion-1.5')
    print ('=== TXT2IMG TEST ===')
    steps=30
    output = next(Txt2Img(model).generate(prompt='banana sushi',
                                          iterations=None,
                                          steps=steps,
                                          step_callback=lambda x: my_callback(x,steps)
                                          )
                  )
    print(f'image={output.image}, seed={output.seed}, steps={output.params.steps}')

if __name__=='__main__':
    main()

@psychedelicious
Copy link
Contributor

@Kyle0654 the signature of step_callback has changed so that will need to be updated on nodes. See example code from Lincoln

@psychedelicious
Copy link
Contributor

thanks, state.step appears to start at -1. what does this mean?

psychedelicious added a commit to psychedelicious/InvokeAI that referenced this pull request Mar 15, 2023
@lstein
Copy link
Collaborator Author

lstein commented Mar 15, 2023

@Kyle0654 the signature of step_callback has changed so that will need to be updated on nodes. See example code from Lincoln

The signature changed when we did the diffusers migration, so it actually a longstanding issue. I would have to defer to @keturn to explain why the first step is -1. It looks like the callback is called once after generation of the initial latent and before the first denoising step (which is step 0).

psychedelicious added a commit to psychedelicious/InvokeAI that referenced this pull request Mar 16, 2023
lstein added 2 commits March 16, 2023 20:05
This PR fixes #2951 and restores the step_callback argument in the
refactored generate() method. Note that this issue states that
"something is still wrong because steps and step are zero." However,
I think this is confusion over the call signature of the callback, which
since the diffusers merge has been `callback(state:PipelineIntermediateState)`

This is the test script that I used to determine that `step` is being passed
correctly:

```

from pathlib import Path
from invokeai.backend import ModelManager, PipelineIntermediateState
from invokeai.backend.globals import global_config_dir
from invokeai.backend.generator import Txt2Img

def my_callback(state:PipelineIntermediateState, total_steps:int):
    print(f'callback(step={state.step}/{total_steps})')

def main():
    manager = ModelManager(Path(global_config_dir()) / "models.yaml")
    model = manager.get_model('stable-diffusion-1.5')
    print ('=== TXT2IMG TEST ===')
    steps=30
    output = next(Txt2Img(model).generate(prompt='banana sushi',
                                          iterations=None,
                                          steps=steps,
                                          step_callback=lambda x: my_callback(x,steps)
                                          )
                  )
    print(f'image={output.image}, seed={output.seed}, steps={output.params.steps}')

if __name__=='__main__':
    main()
```
@psychedelicious psychedelicious force-pushed the bugfix/restore-step-callback branch from a6201bf to 5087f30 Compare March 16, 2023 09:05
@psychedelicious psychedelicious enabled auto-merge (rebase) March 16, 2023 09:06
psychedelicious added a commit that referenced this pull request Mar 16, 2023
- 8693246 add image_to_dataURL util
- 0c26110 make fast latents method
static
- this method doesn't really need `self` and should be able to be called
without instantiating `Generator`
- 2360bfb fix schema gen for
GraphExecutionState
- `GraphExecutionState` uses `default_factory` in its fields; the result
is the OpenAPI schema marks those fields as optional, which propagates
to the generated API client, which means we need a lot of unnecessary
type guards to use this data type. the [simple
fix](pydantic/pydantic#4577) is to add
config to explicitly say all class properties are required. looks this
this will be resolved in a future pydantic release
- 3cd7319 fix step callback and fast
latent generation on nodes. have this working in UI. depends on the
small change in #2957
@psychedelicious psychedelicious merged commit 92721a1 into main Mar 23, 2023
@psychedelicious psychedelicious deleted the bugfix/restore-step-callback branch March 23, 2023 22:32
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.

[bug]: step_callback is not called by refactored base.py

4 participants