Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor entities #586

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

refactor entities #586

wants to merge 14 commits into from

Conversation

hasan7n
Copy link
Contributor

@hasan7n hasan7n commented Apr 29, 2024

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 hasan7n requested a review from a team as a code owner April 29, 2024 12:16
@hasan7n hasan7n requested a deployment to testing-external-code April 29, 2024 12:17 — with GitHub Actions Waiting
Copy link
Contributor

github-actions bot commented Apr 29, 2024

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

cli/cli_tests.sh Show resolved Hide resolved
cli/medperf/commands/list.py Outdated Show resolved Hide resolved
@VukW
Copy link
Contributor

VukW commented Apr 30, 2024

  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, 👍 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

Co-authored-by: Viacheslav Kukushkin <vy.kukushkin@gmail.com>
@hasan7n hasan7n requested a deployment to testing-external-code May 14, 2024 21:48 — with GitHub Actions Waiting
Co-authored-by: Viacheslav Kukushkin <vy.kukushkin@gmail.com>
@hasan7n hasan7n requested a deployment to testing-external-code May 14, 2024 21:55 — with GitHub Actions Waiting
@hasan7n hasan7n requested a review from a team as a code owner May 15, 2024 14:17
@hasan7n hasan7n requested a deployment to testing-external-code May 15, 2024 14:18 — with GitHub Actions Waiting
@hasan7n
Copy link
Contributor Author

hasan7n commented May 15, 2024

@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 hasan7n requested a deployment to testing-external-code May 15, 2024 22:21 — with GitHub Actions Waiting
Copy link
Contributor

@VukW VukW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants