Codecov Report

Merging #646 (cde6109) into main (1c96605) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #646      +/-   ##
==========================================
+ Coverage   86.45%   86.46%   +0.01%     
==========================================
  Files         133      133              
  Lines        6755     6761       +6     
==========================================
+ Hits         5840     5846       +6     
  Misses        915      915              

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

I believe this can be solved inside from_csv itself checking if cls is an Instance or a class.

Thanks for the review, @JoanFM! I think, unfortunately, cls is always a class, even when the classmethod is called from an instance. Therefore, I had a quick chat with Johannes yesterday and he recommended me to follow the approach from jina-ai/serve#4787. Let me know if I’m missing something.

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

multiple inheritance is not so easily supported in Python I think. What is ur super when u call super?

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

multiple inheritance is not so easily supported in Python I think. What is ur super when u call super?

My understanding is that super() picks a certain evaluation order along which it tries to find an __init__ to call, and then it call the first it finds.
But if that __init__ in turn calls super().__init__(**kwargs) then it will keep 'walking' along that evaluation order. So as long as every init in a mixin makes that call it should all work as intended. The mixins just need to be set up to not break this chain.

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

multiple inheritance is not so easily supported in Python I think. What is ur super when u call super?

My understanding is that super() picks a certain evaluation order along which it tries to find an __init__ to call, and then it call the first it finds. But if that __init__ in turn calls super().__init__(**kwargs) then it will keep 'walking' along that evaluation order. So as long as every init in a mixin makes that call it should all work as intended. The mixins just need to be set up to not break this chain.

I agree with Johannes it would be better to add it in the Mixin __init__ but if it is not possible because of inheritance problem we can put in in the DA init

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

multiple inheritance is not so easily supported in Python I think. What is ur super when u call super?

My understanding is that super() picks a certain evaluation order along which it tries to find an __init__ to call, and then it call the first it finds. But if that __init__ in turn calls super().__init__(**kwargs) then it will keep 'walking' along that evaluation order. So as long as every init in a mixin makes that call it should all work as intended. The mixins just need to be set up to not break this chain.

I agree with Johannes it would be better to add it in the Mixin __init__ but if it is not possible because of inheritance problem we can put in in the DA init

Do not put it there. Mixins should be stateless

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

multiple inheritance is not so easily supported in Python I think. What is ur super when u call super?

My understanding is that super() picks a certain evaluation order along which it tries to find an __init__ to call, and then it call the first it finds. But if that __init__ in turn calls super().__init__(**kwargs) then it will keep 'walking' along that evaluation order. So as long as every init in a mixin makes that call it should all work as intended. The mixins just need to be set up to not break this chain.

I agree with Johannes it would be better to add it in the Mixin __init__ but if it is not possible because of inheritance problem we can put in in the DA init

Do not put it there. Mixins should be stateless

But by moving it to the base class we don't avoid the state, we just move it to somewhere else. The only thing that changes is that the assignment happens somewhere where we don't know the thing that we assign...

But not willing to die in this hill, @anna-charlotte you can choose who to listen to ;)

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

multiple inheritance is not so easily supported in Python I think. What is ur super when u call super?

My understanding is that super() picks a certain evaluation order along which it tries to find an __init__ to call, and then it call the first it finds. But if that __init__ in turn calls super().__init__(**kwargs) then it will keep 'walking' along that evaluation order. So as long as every init in a mixin makes that call it should all work as intended. The mixins just need to be set up to not break this chain.

I agree with Johannes it would be better to add it in the Mixin __init__ but if it is not possible because of inheritance problem we can put in in the DA init

Do not put it there. Mixins should be stateless

But by moving it to the base class we don't avoid the state, we just move it to somewhere else. The only thing that changes is that the assignment happens somewhere where we don't know the thing that we assign...

But not willing to die in this hill, @anna-charlotte you can choose who to listen to ;)

What doesn't need state is the Mixin, not the class.

Anyaway, the error came from not passing positional args

Do not add an init to the Mixin. DO that in the init of DocArray Base class

Are we sure about this? It would assign some methods that don't exist there and break separation of concerns. It seems to be accepted practice to let mixins have inits and pass **kwargs between them: https://stackoverflow.com/a/50465583

multiple inheritance is not so easily supported in Python I think. What is ur super when u call super?

My understanding is that super() picks a certain evaluation order along which it tries to find an __init__ to call, and then it call the first it finds. But if that __init__ in turn calls super().__init__(**kwargs) then it will keep 'walking' along that evaluation order. So as long as every init in a mixin makes that call it should all work as intended. The mixins just need to be set up to not break this chain.

I agree with Johannes it would be better to add it in the Mixin __init__ but if it is not possible because of inheritance problem we can put in in the DA init

Do not put it there. Mixins should be stateless

But by moving it to the base class we don't avoid the state, we just move it to somewhere else. The only thing that changes is that the assignment happens somewhere where we don't know the thing that we assign...
But not willing to die in this hill, @anna-charlotte you can choose who to listen to ;)

What doesn't need state is the Mixin, not the class.

I don't agree, the only 'state' we have is which implementation of form_csv we pick, and this is tied to the mixin, not anything else.

I might move the generalization regarding all of the from_xxx classmethods to a new PR, since it has taken me quite long now, so that this one can be merged as soon as it is ready.