-
Notifications
You must be signed in to change notification settings - Fork 37
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
Make CLI support all relevant model types #13
Comments
Hi @kmaziarz I'm an undergrad student from India. I do have experience working on similiar projects and CLI. Can I work on this issue? |
Yes, sure! Sorry for a slow response: the best way of addressing this issue isn't very clear, and so I had to give it a bit of thought. One thing to note is that the underlying model loading utility ( with VaeWrapper(model_dir, **model_kwargs) as model:
(...) which is what we do currently (see e.g. with load_model_from_directory(model_dir, **model_kwargs) as model:
(...) and To return the right wrapper class, we would just need to select one and then pass all the arguments through. We currently have a bunch of filename matching in
What do you think? If it's all too confusing I'm also happy to take a stab at this myself; arguably addressing this issue requires more fiddling with internals than would initially seem... Also pinging @sarahnlewis in case she has any comments. |
@kmaziarz is your suggestion that the model type be found in the contents of the same |
The model class is already being saved in the |
OK, sounds good. |
Thanks for the head start @kmaziarz and @sarahnlewis . I'll get started with the task. |
Hi @kmaziarz so far I have been able to extract the model class from the .pkl file and defined a method which returns which model wrapper class to use based on the model class i.e. Vaewrapper for |
|
@anamika-yadav99: So, summing up, for now let's use the generic way of loading the wrapper for |
Currently all CLI entry points only work for
MoLeRVae
as they useVaeWrapper
, while e.g.molecule_generation sample
could also supportMoLeRGenerator
. While generalizing things, we may also want to rethink the choice to do model type discovery based on filenames (@sarahnlewis may have thoughts on this).The text was updated successfully, but these errors were encountered: