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.

Sign up for GitHub

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

Jump to bottom

feat: Add default UMAP parameters in launch_app() #1224

Merged
merged 30 commits into from
Oct 5, 2023
Merged

feat: Add default UMAP parameters in launch_app() #1224

merged 30 commits into from
Oct 5, 2023

Conversation

pbadhe
Copy link
Collaborator

@pbadhe pbadhe commented Aug 28, 2023

Resolves #1028 when complete

@pbadhe pbadhe changed the title Add default UMAP parameters in launch_app() feat: Add default UMAP parameters in launch_app() Aug 28, 2023
mikeldking
mikeldking reviewed Sep 14, 2023
View reviewed changes
src/phoenix/core/umap_parameters.py Outdated Show resolved Hide resolved
src/phoenix/server/api/context.py Outdated Show resolved Hide resolved
src/phoenix/session/session.py Outdated Show resolved Hide resolved
@pbadhe
Merge branch 'main' into umap_params
c532d04
@pbadhe
Suggested review changes
5e195e4 
Added validation, moved umap_params to pointcloud and removed Optional for self.umap_parameters in context
@mikeldking mikeldking mentioned this pull request Sep 24, 2023
Merged
mikeldking
mikeldking reviewed Sep 24, 2023
View reviewed changes
src/phoenix/session/session.py Outdated Show resolved Hide resolved
src/phoenix/pointcloud/umap_parameters.py Outdated Show resolved Hide resolved
src/phoenix/pointcloud/umap_parameters.py Outdated Show resolved Hide resolved
@mikeldking
Copy link
Contributor

@pbadhe - I pushed the template bootstrapping code via #1459 if you want to take a look. This will allow us to push these values to the client without GraphQL. We will have to still add types on the client side and make it mount from the config singleton as a follow-up. Sorry for the delay.

@pbadhe
Copy link
Collaborator Author

pbadhe commented Sep 24, 2023

Thanks @mikeldking! I will look into it.

@mikeldking
Copy link
Contributor

@pbadhe the template changes are pushed to main. LMK if you need anything else!

pbadhe and others added 5 commits September 25, 2023 22:29
@pbadhe
Merge branch 'main' into umap_params
dc9992b
@pbadhe @mikeldking
CamelCase -> SnakeCase
b71de1a 
Co-authored-by: Mikyo King <mikeldking@gmail.com>
@pbadhe @mikeldking
CamelCase -> SnakeCase all suggested changes
e1f9304 
Co-authored-by: Mikyo King <mikeldking@gmail.com>
@pbadhe
Merge branch 'main' into umap_params
a2f525d
@pbadhe
Default UMAPparam changes
91f5f47 
Removed hardcoded defaults for single source of truth.
@pbadhe
Copy link
Collaborator Author

pbadhe commented Sep 27, 2023

@mikeldking thanks for the server-side template, it helped! I will do the remaining tomorrow, namely processSession() changes, docs updates & type checking.

Apart from this ticket, there's a minor bug in the existing form of UMAP hyperparameters. If you leave nSamples or minDistance empty (I know no one does but still :)), the form is still being submitted and phoenix runs into an error (happens in current release too), whereas the empty field validation is working right only for nNeighbours field. I'll investigate it but if you wish, have a look at the form. If you find the problem, lemme know, nevertheless I will look into it.

@mikeldking
Copy link
Contributor

@mikeldking thanks for the server-side template, it helped! I will do the remaining tomorrow, namely processSession() changes, docs updates & type checking.

Apart from this ticket, there's a minor bug in the existing form of UMAP hyperparameters. If you leave nSamples or minDistance empty (I know no one does but still :)), the form is still being submitted and phoenix runs into an error (happens in current release too), whereas the empty field validation is working right only for nNeighbours field. I'll investigate it but if you wish, have a look at the form. If you find the problem, lemme know, nevertheless I will look into it.

Thanks so much @pbadhe ! I'll take a look hopefully tomorrow. Sorry for the delay!

@pbadhe
Copy link
Collaborator Author

pbadhe commented Sep 28, 2023 •
edited
Loading

@mikeldking I will make it ready for review tomorrow. Thanks!

@pbadhe
Merge branch 'main' into umap_params
d4673ba
@pbadhe
typing hints & process_session()
eb16a0d 
Process_session() error. Logs at -> https://textdoc.co/maM9oCZwexUA4EWQ
@pbadhe
Merge branch 'main' into umap_params
6f2f8e6
@pbadhe
Prettier check
0be3164 
I'd installed the pre-commit hooks, but "black ." was taking too long and checking venv lib packages, so I was doing it manually just on src/*.
@pbadhe pbadhe marked this pull request as ready for review September 29, 2023 20:46
mikeldking
mikeldking reviewed Oct 1, 2023
View reviewed changes
Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

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

@pbadhe this is looking awesome - thanks so much for this! I'll merge this in next week.

app/src/window.d.ts Show resolved Hide resolved
app/src/store/pointCloudStore.ts Outdated Show resolved Hide resolved
src/phoenix/server/templates/index.html Outdated Show resolved Hide resolved
src/phoenix/server/main.py Outdated Show resolved Hide resolved
RogerHYang
RogerHYang reviewed Oct 4, 2023
View reviewed changes
src/phoenix/pointcloud/umap_parameters.py Outdated Show resolved Hide resolved
src/phoenix/pointcloud/umap_parameters.py Outdated Show resolved Hide resolved
src/phoenix/pointcloud/umap_parameters.py Outdated Show resolved Hide resolved
src/phoenix/pointcloud/umap_parameters.py Outdated Show resolved Hide resolved
RogerHYang
RogerHYang reviewed Oct 4, 2023
View reviewed changes
src/phoenix/session/session.py Outdated Show resolved Hide resolved
src/phoenix/session/session.py Outdated Show resolved Hide resolved
src/phoenix/session/session.py Outdated Show resolved Hide resolved
src/phoenix/session/session.py Outdated Show resolved Hide resolved
mikeldking and others added 10 commits October 4, 2023 19:00
@mikeldking @RogerHYang
Update umap_parameters.py
7687b3b 
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
@mikeldking @RogerHYang
Update umap_parameters.py
7aa84a6 
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
@mikeldking @RogerHYang
Update umap_parameters.py
83f6ea5 
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
@mikeldking @RogerHYang
Update src/phoenix/session/session.py
800f85b 
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
@mikeldking @RogerHYang
Update src/phoenix/session/session.py
7ff011f 
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
@mikeldking @RogerHYang
Update src/phoenix/session/session.py
194a1ea 
Co-authored-by: Roger Yang <80478925+RogerHYang@users.noreply.github.com>
@mikeldking
use positional arguments
4a30132
@mikeldking
fix style and imports
fbf87e4
@mikeldking
Merge branch 'main' into umap_params
5b05baa
@mikeldking
Merge branch 'main' into umap_params
4c411be
@mikeldking
Copy link
Contributor

mikeldking
mikeldking approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

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

@mikeldking mikeldking merged commit dce7551 into Arize-ai:main Oct 5, 2023
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 5, 2023
@pbadhe pbadhe deleted the umap_params branch October 7, 2023 23:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RogerHYang RogerHYang RogerHYang left review comments

@mikeldking mikeldking mikeldking approved these changes

Assignees
No one assigned
Labels
None yet
Projects
phoenix
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] provide a way to pas in default UMAP parameters
3 participants
@pbadhe @mikeldking @RogerHYang