Skip to content

dict update syntax improvement to list_models() #1764

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

Closed
wants to merge 1 commit into from
Closed

dict update syntax improvement to list_models() #1764

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ldm/invoke/model_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,14 @@ def list_models(self) -> dict:
else:
status = 'not loaded'

models[name]={
'status' : status,
'description' : description
}
models.update(
{
name: {
Copy link
Contributor

@psychedelicious psychedelicious Dec 11, 2022

Choose a reason for hiding this comment

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

The syntax is a new dict that overwrites the existing keys, so this needs quotes.

Suggested change
name: {
'name': {

Copy link

@mebelz mebelz Dec 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I personally don't understand the original change, or psychedelicious's suggestion.
In my opinion, the original change is switching between equivalent ways of updating a dict (the pre-commit being more readable than the commit).
psychedelicious's suggestion changes the code such that the keys to models are only 'name', and it's constantly overwritten.
Anyway I suggest adding a code comment explaining what is the exact merit of the change, for future reference.

Copy link
Contributor

@tildebyte tildebyte Dec 11, 2022

Choose a reason for hiding this comment

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

@psychedelicious is correct; this code will throw an unknown key exception without the quotes.

Having said that, they're "a horse apiece" - using Dict's update() method is more Pythonic, but I find them equally readable, and equally obvious.

Copy link
Contributor

@psychedelicious psychedelicious Dec 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually @tildebyte I think @mebelz is correct, name is a variable in this context - the name of the model to be updated. I missed this.

'status' : status,
'description' : description
}
}
)
return models

def print_models(self) -> None:
Expand Down