You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I meant to add that each of SmartDatalake and SmartDataframe could have each set of configuration classes, which could share same Abstract Base Class. This could also make it possible to implement methods such as SmartBaseConfiguration.from_dict() which supports Kwargs. Bit like this config class for transformers.
The main problem I see with passing kwargs is that it feels a bit hacky and there is no way of knowing which options are available. With classes and docstrings, it is possible to either view Configuration signature in IDE or with help or in case of Juypter Notebooks with SmartDFConfig?
With 3rd point, I meant perhaps it could be easier to aggregate basic sets of parameters and config options for all LLMs. This will in return make writing tests bit easier in some cases.
At the moment, we support both the implementations and I'm not sure we should drop one or another, but I'm open to any feedbacks from you. That said I can see your point in suggesting to make the second option as default, as it can benefit from a more robust validation.
As of your points:
I don't know if at the moment there are enough differences between SmartDataframe and SmartDataframe to justify a diffrent configuration file for each. But things might change in the future, and it might make sense to have several configuration extending an abstract one.
Nothing to add about the second point
Yeah, 100%. I think instead of acting at the config level, we should also do some changes to the LLM wrappers themselves, making sure that 1) they don't contain business logic apart from connecting to the LLM 2) they have a very similar structure and they accept the same params
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
-
I'm opening this conversation based on the comments in this PR: #551.
@regmibijay mentioned:
So what you suggest is:
Is that correct?
At the moment, we support both the implementations and I'm not sure we should drop one or another, but I'm open to any feedbacks from you. That said I can see your point in suggesting to make the second option as default, as it can benefit from a more robust validation.
As of your points:
Curios to hear your thoughts!
Beta Was this translation helpful? Give feedback.
All reactions