medperf icon indicating copy to clipboard operation
medperf copied to clipboard

refactor entities

Open hasan7n opened this issue 1 year ago • 4 comments

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.

hasan7n avatar Apr 29 '24 12:04 hasan7n

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

github-actions[bot] avatar Apr 29 '24 12:04 github-actions[bot]

  1. I rrreally like how the entities code is simplified. Now it's much easier to be read, thanks a lot man!
  2. In the previous code version TestReport was a bit more separated from other entities as it doesn't include any remote-entities logic. In new code as it inherits from Entity that assumes all ancestors are uploadable, it contain some mock-up methods like get_comms_uploader that are filled with NotImplementedError. Despite of it may look a bit dirty that we have a NIErr functions 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
  3. 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 😅
  4. The only major question IMO is to improve Entity code 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.
  5. I also noted your confusion concerning expected method results types, but also didn't get any reasonable solution while reviewing the code, sorry

VukW avatar Apr 30 '24 22:04 VukW

@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...

hasan7n avatar May 15 '24 14:05 hasan7n

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?

VukW avatar Jun 18 '24 06:06 VukW