Make Artifacts Dynamic on Per Project Basis #574
Conversation
Force *nix style line endings even on Windows
Add wildcard for EOL
Merge upstream
Add Artifact and Group to Admin interface
(not working) Pull in artifact group if set, otherwise use default artifacts.
…dentified_artifacts() Add kwargs to default_identified_artifacts() so we can optionally pass self/project object
…r a given project
…fact/group models
…_decision to match nomenclature in tasks. Remove unnecessary .keys() when iterating through artifacts in tasks Convert for loop to list comprehension, inline variable used once Replace identity comprehension with call to collection constructor (improves code clarity) Inline artifact_values that is immediately returned
…ettings to settings
miqa/core/models/group.py
Outdated
| from django.db import models | ||
|
|
||
|
|
||
| class Group(models.Model): |
There was a problem hiding this comment.
Let's name this model ArtifactGroup
There was a problem hiding this comment.
@zachmullen - I had initially named this ArtifactGroup but realized there were multiple items that could use similar grouping. Rather than creating a separate model for each type of item I created the generic Group.
Other objects I thought could be similarly grouped (/made dynamically editable) in the future include:
- evaluation models (core\project.py, learning/evaluation_models.py)
- scan types (core\scan.py)
- decision choices (core\scan_decision.py)
- columns (learning\nn_training.py)
If you don't see any of these as likely candidates to become dynamic in the future / would rather have a separate model for each, I can definitely refactor back to ArtifactGroup, just lmk.
There was a problem hiding this comment.
I think a separate model for each would be preferable, otherwise it would be possible to group multiple different domain objects within the same Group.
There was a problem hiding this comment.
Having those things dynamically editable sounds appealing to me.
There was a problem hiding this comment.
@zachmullen , are you recommending that we shouldn't use the Group model and instead be specific to only artifacts, then create other groups for other changeable features? I like that a project can have a particular Group name, which can specify Artifcacts now and later specify the other options Dave mentioned (scans, learning columns, etc.) Maybe I am just confused, but I liked the design as originally submitted. Maybe I just need a bit of explanation. thanks.
There was a problem hiding this comment.
Yes, that is what I was proposing. If we actually want multiple different types of things to be grouped together, then perhaps a generic Group is a good model, but that didn't seem like the design intent, as the field on the Project model was named artifact_group. I assumed we would only ever want to group things together of the same domain type.
There was a problem hiding this comment.
@zachmullen , Thanks. It is my opinion that we would want to use the Group model to select sets of different things to go with projects. In other words, the same way we have an Artifact model now with a group attribute, so we can build lists of Artifcats for each project , I think we might want to have the name of the model and its Evaluation columns (the outputs of the learning model) done similarly.
I don't want to embark on this approach immediately across the board, but I'd like to leave room for generalizing the application of MIQA to a learning model designed for my Lung Cancer application, for example.
There was a problem hiding this comment.
If we do want multiple types of domain objects to be put into the same group, I'd still suggest we change the name to e.g. ProjectOptionsGroup, to avoid collision with the existing Group model from django-contrib.
There was a problem hiding this comment.
@zachmullen - Definitely see how the artifact_group on Project could be misleading. If I understand correctly Django can support multiple fields to the same model with different names (e.g. scan_group also pointing to the Group model).
Changing the name from Group makes sense to me. Wondering if we should make it ProjectOptionsGroup or OptionsGroup to allow it to be used more generically for settings that could occur at other levels (e.g. global or perhaps per experiment)? I don't have a strong opinion on this, just wanted to raise the Q.
There was a problem hiding this comment.
Group has been recreated as SettingsGroup (meant it to be OptionsGroup but if it is all the same will leave as is, otherwise, easy change to OptionsGroup).
miqa/core/models/scan_decision.py
Outdated
| @@ -37,8 +26,13 @@ class ArtifactState(Enum): | |||
| ABSENT = 0 | |||
| UNDEFINED = -1 | |||
|
|
|||
| def default_identified_artifacts(scan_project_artifacts = ''): | |||
There was a problem hiding this comment.
I think rather than use this as the default value of user_identified_artifacts, we should redesign this a bit. Namely, let's make that column nullable, and if it's null, we can construct this value on the fly when we want to render it.
We can add the following to the Project model:
@property
def artifacts(self) -> list[str]:
if self.artifact_group:
return [artifact.name for artifact in self.artifact_group.artifacts]
else:
return settings.DEFAULT_ARTIFACTSand move the rest of the logic into the rendering layers (either the REST API or the front-end) since it's just string munging for display purposes.
There was a problem hiding this comment.
Removing default_identified_artifacts and it's string munging is outside what I could accomplish atm. Totally good if you don't want to merge without, as mentioned elsewhere have to move onto other lung project priorities.
miqa/core/rest/other_endpoints.py
Outdated
| @@ -27,7 +27,7 @@ class MIQAConfigView(APIView): | |||
| def get(self, request): | |||
| return Response( | |||
| { | |||
| 'artifact_options': [key for key in default_identified_artifacts()], | |||
| 'artifact_options': list(default_identified_artifacts()), | |||
There was a problem hiding this comment.
| 'artifact_options': list(default_identified_artifacts()), |
Let's remove this field, so that this is always read from the properties of the project.
There was a problem hiding this comment.
I ran into some weird issues implementing this on the Vue side, so I've reverted to using default_identified_artifacts for now. I'd love to come back to it at some point, but need to keep moving on lung's project. Understand if you don't want to merge without this implemented.
Make description optional. Co-authored-by: Zach Mullen <[email protected]>
Add related_name to artifact Co-authored-by: Zach Mullen <[email protected]>
Used related_name to simplify pulling associated artifacts Co-authored-by: Zach Mullen <[email protected]>
…oject rest endpoint.
|
FYI Update: Ran into some issues and needed to rebuild my environment. Getting close and should have some commits to finish request changes and clean things up coming this week. |
…Decision, Tasks Move DEFAULT_ARTIFACTS out of ScanDecision and into settings.
# Conflicts: # miqa/core/admin.py # miqa/core/models/__init__.py # miqa/core/models/artifact.py # miqa/core/models/project.py # miqa/core/models/scan_decision.py # miqa/core/rest/project.py # web_client/src/components/DecisionButtons.vue # web_client/src/types.ts
|
Thanks. Agreed.
… On Aug 31, 2022, at 1:13 PM, Zach Mullen ***@***.***> wrote:
@zachmullen commented on this pull request.
In miqa/core/models/group.py:
> @@ -0,0 +1,17 @@
+from uuid import uuid4
+from django.db import models
+
+
+class Group(models.Model):
If we do want multiple types of domain objects to be put into the same group, I'd still suggest we change the name to e.g. ProjectOptionsGroup, to avoid collision with the existing Group model from django-contrib.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
|
|
I completely rewrote the dynamic settings functionality from the ground up, so I'm going to close this PR. The rewritten functionality can be found in: https://github.com/davidshq/miqa/tree/abstract-with-enums If there is interest in merging it, let me know and I can create a new PR. The new functionality uses a single model (Setting) that can support almost all types of settings. There is a I've focused on artifacts, models, file type to model mappings, etc. |
Artifacts were previously set in code. This PR makes Artifacts dynamically selectable on a per project basis.
(This dynamism is limited to the main application, it does not extend to the learning/neural network portion)
This was accomplished by adding two models to MIQA in Django.
default_evaluation_model_mappinginproject.pyandSCAN_TYPESinscan.py)Each Project can be associated with a Group (of artifacts). This group of artifacts is then displayed within the Vue UI (for selection by QA) and also utilized during
tasks.pyimport.