search

Bruno-val-bus / student-helper

0 stars 0 forks source link

replace Pydantic parser with Schematic parser to make llama3:8b work #7

Closed ernOho closed 3 months ago

ernOho commented 4 months ago
ernOho commented 3 months ago

Ill have a look into it tomorrow

Bruno-val-bus commented 3 months ago

Ill have a look into it tomorrow

I’ve been implementing a proposal. I’d suggest we discuss it first before you do any rework.

ernOho commented 3 months ago

Alright awesome @Bruno-val-bus . Then I will have to wait for your proposal, test it with the local model before I can create another merge request relating to issue #6 , which in theory is also solved

ernOho commented 3 months ago

@Bruno-val-bus , I can't verify this pull request, as I created it.

Either you can review or accept it, or you need to create a new one I think.

Bruno-val-bus commented 3 months ago

@Bruno-val-bus , I can't verify this pull request, as I created it.

Either you can review or accept it, or you need to create a new one I think.

Will review and accept then. I noticed you used an abstract class for the configuration. I don’t see the configuration class behaving differently, so I’m not sure if an interface pattern is necessary here. Was there a special reason for it?

ernOho commented 3 months ago

Yeahr fair. No real special reason for it, exept for trying to follow the SOLID design principles (specifically Dependency Inversion principle). But yeah overkill or not implemented properly

Bruno-val-bus commented 3 months ago

Definitely a good idea to keep that design principle in mind. Whenever i decide wether to use an interface or not, I ask if the class is behaving differently or will only handle different data . For the configuration class i think it will merely do the latter. If you think the same, i will just keep the class and merge?