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

Support multi-inheritance classes #143

Closed
florianblume opened this issue Jul 1, 2022 · 3 comments
Closed

Support multi-inheritance classes #143

florianblume opened this issue Jul 1, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@florianblume
Copy link

🚀 Feature request

As far as I can tell, currently there's not support for multi-inheritance classes (but of course let me know if I'm wrong or implementing something wrong). The following example throws an error:

from jsonargparse import ArgumentParser
import yaml

class ModelBaseClass():
    def __init__(self, batch_size: int=0):
        self.batch_size = batch_size

class ImageModel(ModelBaseClass):
    def __init__(self, image_size: int=0, **kwargs):
        super().__init__(**kwargs)
        self.image_size = image_size

class ClassModel(ModelBaseClass):
    def __init__(self, num_classes: int=0):
        self.num_classes = num_classes

class ImageClassModel(ImageModel, ClassModel):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)

parser = ArgumentParser(
    prog='app',
    description='Description for my app.'
)

parser.add_subclass_arguments(ModelBaseClass, 'model')

model = yaml.safe_dump({
    'class_path': '__main__.ImageClassModel',
    'init_args': {
        'image_size': 5,
        'num_classes': 10
    }
})

print(parser.parse_args([f'--model={model}']))

It used to work in the fork I made off of version 4.8 but somehow stopped to work now.

Motivation

I guess the example makes it pretty obvious why this is a neat feature, it just makes sense to separate the classes like this (maybe you have a classifier that is not classifying images) but also compose them again.

Pitch

It would be cool if it would work again like in version 4.8 :) I'd also happy to help out if it's a simpler fix but it might be faster/nicer coded if you do it yourself ;)

Alternatives

Not really any that I can think of.

@florianblume florianblume added the enhancement New feature or request label Jul 1, 2022
@mauvilsa mauvilsa added bug Something isn't working and removed enhancement New feature or request labels Jul 4, 2022
@mauvilsa
Copy link
Member

mauvilsa commented Jul 4, 2022

This worked before, making it a regression. So changed the issue from enhancement to bug.

@mauvilsa
Copy link
Member

mauvilsa commented Jul 4, 2022

This is fixed in commit 57fe548. Note that there is a mistake in the code snippet you included above. Creating an instance of ImageClassModel will not call ModelBaseClass.__init__, which means that batch_size will not be set. This can be observed by running list(ImageClassModel().__dict__.keys()) which gives ['num_classes', 'image_size']. The problem is that ClassModel is missing super().__init__(**kwargs). Without fixing that code, jsonargparse will work the same way, not accepting the parameter batch_size.

@mauvilsa mauvilsa closed this as completed Jul 4, 2022
@florianblume
Copy link
Author

Nice, works perfectly. Thanks about mentioning the bug in the example script, it's not actual code but I wrote it from scratch, that's probably why this error is in there.

I'd love to directly implement a fix next time but every time I look at your changes I get that I don't understand the project well enough for that, yet... So thanks a lot for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants