Refactoring/experiments module #180
Conversation
cristian-tamblay
requested review from
pbadillatorrealba and
cristian-tamblay
There was a problem hiding this comment.
The changes look good!
I think I have roughly understood the new structure and when to use pages, hooks, layouts, etc... but I would find it great if you could document these (if it doesn't already exist).
I left a lot of comments, mainly doubts and suggestions for changes, but in general the changes look great.
In addition, I moved the base to dev, so that the changes are merged directly. I think this is better than having them go to an intermediate branch, since so far, I think they should not collide with any other project.
I do want to say that I won't be revising such a long pr again, because:
- As there is a lot to review, it is difficult to focus and understand the changes. Most probably I might have missed some bugs.
- I got ultra tired and right now i can barely think 🫠😅
Next time, just modify a limited amount of features in each pr and upload them for review. And then iterate with this idea :)
| enqueueSnackbar("Experiment successfully created.", { | ||
| variant: "success", | ||
| }); | ||
| onSuccess && onSuccess(); |
There was a problem hiding this comment.
onSuccess could be optional?
| } | ||
| } | ||
| }; | ||
| return { uploadNewExperiment }; |
There was a problem hiding this comment.
It is standarized that the hooks must return objects?
| ); | ||
|
|
||
| const experimentId = response.id; | ||
| await uploadRuns(experimentId); |
There was a problem hiding this comment.
What happens if the server fails to create a run?
Shouldn't we do a kind of rollback?
I think we need to think about this behavior a bit more, since, at least for me, the idea is that the front end should have as little logic as possible. Maybe we could redo the run requests as sub-resources of experiments and from that, handle errors.
There was a problem hiding this comment.
In the meantime, I would do that:
If any run fails, have an exception raised in uploadRuns.
Let the experiment finish creating, but throw a yellow snackbar in case any flag indicates that it failed to create at least one run.
| <IconButton | ||
| edge="start" | ||
| color="inherit" | ||
| onClick={handleClose} | ||
| sx={{ display: { xs: "flex", sm: "none" } }} | ||
| > | ||
| <CloseIcon /> | ||
| </IconButton> |
There was a problem hiding this comment.
I don't know if it's just me, but I couldn't see this icon.
| * This pattern is called compound components, you can read more about it here: | ||
| * https://kentcdodds.com/blog/compound-components-with-react-hooks |
| const isEmpty = datasets.length === 0 && !loading; | ||
| return ( | ||
| <ExperimentsCreateDatasetStepLayout isEmpty={isEmpty}> | ||
| <DataGrid |
There was a problem hiding this comment.
If we follow the layout structure, shouldn't this table be in the layout (passing the states as arguments)?
| <React.Fragment> | ||
| <Alert severity="warning" sx={{ mb: 2 }}> | ||
| <AlertTitle> | ||
| There is no datasets associated to the selected task. |
There was a problem hiding this comment.
Further on, datasets will not be associated with a defined task.
We could switch to :
"There is no dataset currently stored in the database".
| /** | ||
| * This component renders a table to display the models that are currently in the experiment | ||
| * @param {object} newExp object that contains the Experiment Modal state | ||
| * @param {function} setNewExp updates the Eperimento Modal state (newExp) |
There was a problem hiding this comment.
| * @param {function} setNewExp updates the Eperimento Modal state (newExp) | |
| * @param {function} setNewExp updates the Experiment Modal state (newExp) |
| const [schema, setSchema] = useState({}); | ||
| const [loading, setLoading] = useState(false); | ||
|
|
||
| const getModel = useCallback(async () => { |
There was a problem hiding this comment.
Is this a cache?
| const filteredDatasets = taskName | ||
| ? datasets.filter((dataset) => dataset.task_name === taskName) | ||
| : datasets; | ||
| setDatasets(filteredDatasets); |
There was a problem hiding this comment.
Perhaps this function could be generalized by making that if the filter is null, all the datasets are fetched indistinctly.
There was a problem hiding this comment.
This is with the idea that later it can be reused in the datasets page.
Summary
This pull request addresses the refactoring of the experiments module in the DashAI software. The key adjustment involves a comprehensive reorganization of the folder structure, adopting a clean architecture paradigm. Modules are now consolidated within the 'pages' folder, containing the entry page, components, hooks, and constants tailored to each module. The aim of this modification is to notably enhance code readability and maintainability by adopting a more structured and modular approach.
Type of change
Changes
Elements were decoupled to diminish dependencies, and the core logic underwent reorganization into React hooks, enabling the efficient reuse of stateful logic across components. New components were introduced to achieve a more modular and visually focused design, following a defined naming convention. Additionally, all the constant values were brought together in one place, making it easier to maintain and manage.
How to Test
It has to work the same as the 'develop' branch.
Notes
This is how it looks: