Skip to content

List names, demucs cleanup#158

Open
zdevito wants to merge 1 commit intogh/zdevito/11/basefrom
gh/zdevito/11/head
Open

List names, demucs cleanup#158
zdevito wants to merge 1 commit intogh/zdevito/11/basefrom
gh/zdevito/11/head

Conversation

@zdevito
Copy link
Contributor

@zdevito zdevito commented Nov 20, 2020

Stack from ghstack:

  • [wip] some clean and code to package models #161 [wip] some clean and code to package models

  • List names, demucs cleanup #158 List names, demucs cleanup

  • Separate listing the names of modules from actually loading one.

  • Module import for a model sometimes takes time, so it is helpful
    to be able to filter by module name before importing the module souce.

  • Also allows manually looking up module load_model('demucs')

  • Fixes an issue where demucs get_module was not returning a module.

* Separate listing the names of modules from actually loading one.
* Module import for a model sometimes takes time, so it is helpful
  to be able to filter by module name _before_ importing the module souce.
* Also allows manually looking up module `load_model('demucs')`
* Fixes an issue where demucs get_module was not returning a module.

[ghstack-poisoned]
zdevito added a commit that referenced this pull request Nov 20, 2020
* Separate listing the names of modules from actually loading one.
* Module import for a model sometimes takes time, so it is helpful
  to be able to filter by module name _before_ importing the module souce.
* Also allows manually looking up module `load_model('demucs')`
* Fixes an issue where demucs get_module was not returning a module.

ghstack-source-id: c6b7dfc
Pull Request resolved: #158
@zdevito zdevito requested a review from wconstab November 20, 2020 21:45
Model.name = model_name
models.append(Model)
return models
return (load_model(model_name) for model_name in model_names())
Copy link
Contributor

Choose a reason for hiding this comment

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

i thought you wanted to decouple listing of models from loading them?

Copy link
Contributor

Choose a reason for hiding this comment

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

or did you just want to decouple loading a model by name from having to load all the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made module_names public so if you wanted to separate the process you can. I didn't want to change the existing API for the case where you will just load everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

might still be useful to have list_models return a list of string names of directories in case one model isn't importable and it forces you to mv the dir out before anything works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I am following. One thing to keep in mind is that load_model handles the import, so if you don't try to load a model it won't import it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i wrote my latest comment without seeing yours, that makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants