refactor entities
Refactor entities and use inheritance to not repeat code. This is needed since for FL support we are adding 4 new entities with similar interface.
This PR also removes the complexity and notion of local-only for listing entities, and defines an unregistered notion
One downside for this change is intellisense in code editors is now broken. To solve this (in another PR), we can use type hints when retrieving entities, and refactor the inheritance of schemas.
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅
- I rrreally like how the entities code is simplified. Now it's much easier to be read, thanks a lot man!
- In the previous code version
TestReportwas a bit more separated from other entities as it doesn't include any remote-entities logic. In new code as it inherits fromEntitythat assumes all ancestors are uploadable, it contain some mock-up methods likeget_comms_uploaderthat are filled withNotImplementedError. Despite of it may look a bit dirty that we have aNIErrfunctions in the running code, I personally like this solution more than previous one; it seems cleaner to just "close" some unused remote methods rather then separate TestReport from other entities on the architecture & inheritance level. So, :+1: for the new solution - I didn't review test code quite well, sorry. I just hope it works. It's a cruel irony you have to spend so much efforts on fixing tests (I'm pretty sure it wasn't easy) and I cannot appreciate it as reading pytest code is too difficult 😅
- The only major question IMO is to improve
Entitycode even more and declare all required fields / properties there (in constructor?):id/generated_uid/for_test, maybe smth else. The goal is to show IDE that Entity always have these fields and it's safe to refer to them. - I also noted your confusion concerning expected method results types, but also didn't get any reasonable solution while reviewing the code, sorry
@VukW
Thanks for the great comments. One major thing I pushed is that I went further and made the TestReport have the common attributes of other entities (id, created_at, owner, ....) so that we get rid of MedPerfbaseschema vs medperfschema, hence we don't make the inheritence complicated...
Regarding expected methods results type and intellisense, it seems to work when we do something like dataset: Dataset = Dataset.get(...). It's an overkill to make such a change in all the code so I think while we develop other stuff in other PRs we can start using type hints...
Auth test still fails because of webdriver-manager requires Chrome == 125 (by default), while new version installs Chrome == 127(?)
https://github.com/mlcommons/medperf/actions/runs/9464826888/job/26072984898?pr=586
Can we solve this in a separate PR and merge this one?