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

Support adding/removing pipelines/post-processors #561

Merged
merged 18 commits into from
May 25, 2023

Conversation

nandhinibsn
Copy link
Contributor

@nandhinibsn nandhinibsn commented May 2, 2023

Resolve #

Description:

Checklist:

You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.

  • ISSUE NUMBER. You linked the issue number (Ex: Resolve #XXX).
  • PRE-COMMIT. You ran pre-commit on all commits, or else, you
    ran pre-commit run --all-files at the end.
  • USER CHANGES. The changes are added to CHANGELOG.md and the documentation, if they impact
    our users.
  • DEV CHANGES.
    • Update the documentation if this PR changes how to develop/launch on the app.
    • Update the README files and our wiki for any big design decisions, if relevant.
    • Add unit tests, docstrings, typing and comments for complex sections.

@nandhinibsn nandhinibsn self-assigned this May 2, 2023
@nandhinibsn nandhinibsn force-pushed the nandhini/edit-pipeline-postprocessors branch from fb20640 to 47da309 Compare May 4, 2023 15:37
Copy link
Contributor

@JosephMarinier JosephMarinier left a comment

Choose a reason for hiding this comment

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

I suggest moving the <FormGroup> inside the pipelines.map() so the dividers can span all the way to the Paper borders.

webapp/src/pages/Settings.tsx Outdated Show resolved Hide resolved
webapp/src/pages/Settings.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@JosephMarinier JosephMarinier left a comment

Choose a reason for hiding this comment

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

Same thing for postprocessors.

webapp/src/pages/Settings.tsx Outdated Show resolved Hide resolved
webapp/src/pages/Settings.tsx Outdated Show resolved Hide resolved
@nandhinibsn nandhinibsn force-pushed the nandhini/edit-pipeline-postprocessors branch from 47da309 to 593cbbf Compare May 8, 2023 14:30
defaultConfig.pipelines![0].postprocessors
)?.map((postprocessor, index) => (
<React.Fragment key={index}>
<IconButton
Copy link
Contributor

@JosephMarinier JosephMarinier May 12, 2023

Choose a reason for hiding this comment

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

Now that you moved the delete button definition up (and use position: "absolute"), if you define a custom metric, the remote field gets in front of the button, making clicking it difficult. The delete button should be defined after (below) the fields. Don't forget to try your changes!

alignItems="center"
>
{displaySectionTitle("General")}
<IconButton
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to position the delete button the same way for the pipelines as it is for the postprocessors? So it's uniform.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did some styling on the delete button as per your comment.

I still see a bigger gap around the pipeline delete button than there is around the postprocessor delete button.
239018517-6ff8fc5b-20d1-4609-b664-ee78f6fcc0e3

@nandhinibsn
Copy link
Contributor Author

I did some styling on the delete button as per your comment. Let me know what do you think of it.

image

nandhinibsn and others added 6 commits May 18, 2023 11:47
so it gets the focus when adding a post-processor
so it is clear that you are not limited to the two proposed options (Thresholding and TemperatureScaling)
@JosephMarinier JosephMarinier changed the title Add and delete pipeline section Support adding/removing pipelines/post-processors May 25, 2023
@JosephMarinier JosephMarinier enabled auto-merge (squash) May 25, 2023 14:44
@JosephMarinier JosephMarinier merged commit 797ba66 into main May 25, 2023
2 checks passed
@JosephMarinier JosephMarinier deleted the nandhini/edit-pipeline-postprocessors branch May 25, 2023 15:02
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