Skip to content
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.

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

Refrain from writing deprecated legacy options to invokeai.yaml #4190

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

lstein
Copy link
Collaborator

@lstein lstein commented Aug 7, 2023

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

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

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because:

Have you updated all relevant documentation?

  • Yes
  • No

Description

When migrating the invokeai.init legacy configuration file to the new .yaml format, do not copy any deprecated fields. This PR also activates the Pydantic validate_assignment behavior which raises an exception when an instantiated config object is updated with invalid values.

Related Tickets & Documents

  • Related Issue #
  • Closes #

QA Instructions, Screenshots, Recordings

Added/updated tests?

  • Yes
  • No : please replace this line with details on why tests
    have not been included

[optional] Are there any post deployment tasks we need to perform?

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

I'm approving this, assuming it resolves the issue, but I did raise some concerns as comments, which may be relevant.

invokeai/app/services/config.py Show resolved Hide resolved
@psychedelicious
Copy link
Collaborator

Ah, I missed your comments in the PR itself about validate_assignment, cool.

I'd still like to understand why this PR couldn't just add that as a safeguard, and then remove the restore field from the config model. If we aren't going to reference it, why do we need to maintain that value?

@lstein
Copy link
Collaborator Author

lstein commented Aug 8, 2023

Ah, I missed your comments in the PR itself about validate_assignment, cool.

I'd still like to understand why this PR couldn't just add that as a safeguard, and then remove the restore field from the config model. If we aren't going to reference it, why do we need to maintain that value?

I will remove the deprecated fields and see if it causes any issues. As I recall, the presence of additional fields in invokeai.yaml were causing exceptions, and this was without setting Extra.ignore. But the code has changed a lot since then.

@psychedelicious
Copy link
Collaborator

I've marked the conversations resolved so we can get this important fix in, but I think there's some room to improve how we handle InvokeAIAppConfig to better leverage the data integrity confidence afforded by Pydantic.

@lstein
Copy link
Collaborator Author

lstein commented Aug 8, 2023

I've removed the deprecated options from config, tested both install and web server, and all seems well.

@hipsterusername hipsterusername force-pushed the bugfix/dont-migrate-deprecated-legacy-options branch from e3aec04 to d4da77f Compare August 8, 2023 15:19
@hipsterusername hipsterusername merged commit e77400a into main Aug 8, 2023
8 checks passed
@hipsterusername hipsterusername deleted the bugfix/dont-migrate-deprecated-legacy-options branch August 8, 2023 15:33
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