Added filtering option for entities listing #513
Conversation
VukW
had a problem deploying
to
testing-external-code
GitHub Actions
Failure
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
| @@ -5,15 +5,14 @@ | |||
| class Entity(ABC): | |||
| @abstractmethod | |||
| def all( | |||
| cls, local_only: bool = False, comms_func: callable = None | |||
| cls, local_only: bool = False, filters: dict = None | |||
There was a problem hiding this comment.
This fix was not connected to ls valid feature, just a bugfix: making abstract function signature relevant to a real implementations
| @@ -105,3 +106,6 @@ def default_status(cls, v): | |||
| if v is not None: | |||
| status = Status(v) | |||
| return status | |||
|
|
|||
| class DeployableEntity(DeployableSchema, Entity): | |||
| pass | |||
There was a problem hiding this comment.
This "implementation" is necessary at two places:
- Entity list command - to declare that applicable objects should be both DeployableSchema and Entity.
- for Entity list test purposes
However, placing implementation here breaks code design a bit: it makeschemas.pydependable frominterface.py. Maybe it's worth to move this class definition to other place?
There was a problem hiding this comment.
I see your point. Maybe this could live in the interface file instead? I also think that, just for clarity, entities that apply to this description should inherit from this DeployableEntity class.
|
|
||
| # generate_cube(id=103, is_valid=True, owner=12345), | ||
| # generate_cube(id=104, is_valid=False, owner=12345), | ||
| ] |
There was a problem hiding this comment.
The current behavior is not to filter local cubes by owner. I didn't change it, instead I anchored it in this test case. In future, once we elaborate & implement new ls behavior, these tests have to be changed
VukW
had a problem deploying
to
testing-external-code
GitHub Actions
Failure
| mocker.patch.object(entity_object, "display_dict", side_effect=display_dicts) | ||
| mocker.patch("medperf.commands.list.get_medperf_user_data", return_value={"id": 1}) | ||
|
|
||
| # spies | ||
| generated_entities = [entity_object for _ in display_dicts] | ||
| print([e.is_valid for e in generated_entities]) |
There was a problem hiding this comment.
This print is probably not needed
| @pytest.mark.parametrize("local_only", [False, True]) | ||
| @pytest.mark.parametrize("mine_only", [False, True]) | ||
| @pytest.mark.parametrize("valid_only", [False, True]) | ||
| def test_run_list_mlcubes(mocker, comms, ui, local_only, mine_only, valid_only): |
There was a problem hiding this comment.
This test is way too complex. Is there a way we can split it into multiple tests? It is considered best practice to have atomic, DAMP unit tests
| @@ -105,3 +106,6 @@ def default_status(cls, v): | |||
| if v is not None: | |||
| status = Status(v) | |||
| return status | |||
|
|
|||
| class DeployableEntity(DeployableSchema, Entity): | |||
| pass | |||
There was a problem hiding this comment.
I see your point. Maybe this could live in the interface file instead? I also think that, just for clarity, entities that apply to this description should inherit from this DeployableEntity class.
Solution for #496 : add
--validflag for entity listing:Waits for #509 to implement
is_validflag forResultentity