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

DagsHub Logger: Fix unsupported metric formats for MLflow, Add example notebook #915

Merged
merged 17 commits into from
Aug 8, 2023
Merged

DagsHub Logger: Fix unsupported metric formats for MLflow, Add example notebook #915

merged 17 commits into from
Aug 8, 2023

Conversation

nirbarazida
Copy link
Contributor

MLflow doesn't support some of the formats YOLO-NAS uses, so we either reformat them or raise an expiration that this format is not supported and will not be logged. This way, if YOLO-NAS uses a metric we didn't encounter during our tests and is not supported by MLflow, the training process will not stop, and the user will get an informative error message about it.

@dagshub
Copy link

dagshub bot commented May 4, 2023

Join the discussion on DagsHub!

@deanp70
DagsHub Logger: Fix the error when encounter the unsupported type for…
369ec4b 
… mlflow. Add a Colab Notebook using DagsHub Logger
ofrimasad
ofrimasad reviewed May 28, 2023
View reviewed changes
Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

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

looks great.
one minor comment

src/super_gradients/common/sg_loggers/dagshub_sg_logger.py Outdated Show resolved Hide resolved
@deanp70
Copy link
Contributor

deanp70 commented Jun 11, 2023

Hey @ofrimasad, I fixed the issue you mentioned, can this be merged in now?

Thanks,
Dean

@deanp70
Copy link
Contributor

deanp70 commented Jun 26, 2023

@BloodAxe we reviewed and fixed your comments on the unsupported symbols. Please let me know if there's anything else, or if we can go ahead and merge this in. Thanks in advance! 🙏

@deanp70
Copy link
Contributor

deanp70 commented Jul 11, 2023 •
edited
Loading

@BloodAxe / @ofrimasad Fixed both comments. Is it possible to merge now?

Also, could I hope it's ok with you, but if you do find any additional changes – would it be possible to review the entire contribution, since doing it a couple of fixes at a time is much more time-consuming?

Thanks in advance, appreciate the consideration 🙏

@nirbarazida
Copy link
Contributor Author

Hi @ofrimasad @BloodAxe

We've fixed all comments and ran a few tests to check the "happy path" works and other edge cases.
Is it possible to merge now?

@nirbarazida
Copy link
Contributor Author

Hey @BloodAxe @ofrimasad
Did you have a chance to review the changes? We want to promote the integration on our channels but waiting for the fixes to be merged.

BloodAxe
BloodAxe approved these changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@BloodAxe BloodAxe left a comment

Choose a reason for hiding this comment

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

Sorry, getting this PR took way too long.
LGTM

@BloodAxe BloodAxe merged commit f24954c into Deci-AI:master Aug 8, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrimasad ofrimasad ofrimasad left review comments

@timho102003 timho102003 timho102003 left review comments

@deanp70 deanp70 deanp70 left review comments

@BloodAxe BloodAxe BloodAxe approved these changes

@shaydeci shaydeci Awaiting requested review from shaydeci shaydeci is a code owner

@Louis-Dupont Louis-Dupont Awaiting requested review from Louis-Dupont

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nirbarazida @deanp70 @BloodAxe @ofrimasad @timho102003 @ranrubin