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

Add support for MLFlow logger #1847

Merged
merged 19 commits into from
May 1, 2024

Conversation

DoMaLi94
Copy link
Contributor

@DoMaLi94 DoMaLi94 commented Mar 12, 2024

πŸ“ Description

This PR integrates MLFlow into Anomalib.
Addresses #1206

✨ Changes

Select what type of change your PR is:

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • πŸ”¨ Refactor (non-breaking change which refactors the code base)
  • πŸš€ New feature (non-breaking change which adds functionality)
  • πŸ’₯ Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • πŸ“š Documentation update
  • πŸ”’ Security update

βœ… Checklist

Before you submit your pull request, please make sure you have completed the following steps:

  • πŸ“‹ I have summarized my changes in the CHANGELOG and followed the guidelines for my type of change (skip for minor changes, documentation updates, and test enhancements).
  • [x ] πŸ“š I have made the necessary updates to the documentation (if applicable).
  • πŸ§ͺ I have written tests that support my changes and prove that my fix is effective or my feature works (if applicable).

For more information about code review checklists, see the Code Review Checklist.

Copy link

Check out this pull request onΒ  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added Dependencies Pull requests that update a dependency file Notebooks labels Mar 12, 2024
@DoMaLi94 DoMaLi94 closed this Mar 12, 2024
@DoMaLi94 DoMaLi94 reopened this Mar 12, 2024
@DoMaLi94
Copy link
Contributor Author

I saw that DCO failed. I'm not sure if any of the solutions under "Details" are applicable. Could someone kindly provide some guidance or recommendations on how to address this issue?

@samet-akcay
Copy link
Contributor

I saw that DCO failed. I'm not sure if any of the solutions under "Details" are applicable. Could someone kindly provide some guidance or recommendations on how to address this issue?

Hi @DoMaLi94, it's passing now. Thanks for creating this PR.

The team will review it, and get back to you

@samet-akcay samet-akcay mentioned this pull request Mar 18, 2024
9 tasks
Copy link
Contributor

@samet-akcay samet-akcay left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR! I have a single comment concerning the import error.

src/anomalib/loggers/mlflow.py Outdated Show resolved Hide resolved
@samet-akcay samet-akcay added Requires Changes Reviewers request changes, which should be addressed by the PR maker Feature and removed Dependencies Pull requests that update a dependency file Notebooks labels Mar 18, 2024
@github-actions github-actions bot added Dependencies Pull requests that update a dependency file Notebooks labels Mar 23, 2024
Signed-off-by: Dominik Linsmayer <50681661+DoMaLi94@users.noreply.github.com>
Signed-off-by: Dominik Linsmayer <50681661+DoMaLi94@users.noreply.github.com>
Signed-off-by: Dominik Linsmayer <50681661+DoMaLi94@users.noreply.github.com>
@DoMaLi94
Copy link
Contributor Author

After my new commits DCO fails again. Is it safe to follow the instructions under Details and perform a rebase in this state? My newer commits now include a Signed-off-by line :).

@samet-akcay samet-akcay added this to the v1.1.0 milestone Mar 23, 2024
@samet-akcay
Copy link
Contributor

After my new commits DCO fails again. Is it safe to follow the instructions under Details and perform a rebase in this state? My newer commits now include a Signed-off-by line :).

it's alright, I've now set DCO to pass and triggered the CI.

@samet-akcay samet-akcay removed Dependencies Pull requests that update a dependency file Notebooks labels Mar 25, 2024
requirements/loggers.txt Outdated Show resolved Hide resolved
….0.0 to match the requirements from lightning in requirements/core.txt

Signed-off-by: Dominik Linsmayer <50681661+DoMaLi94@users.noreply.github.com>
Signed-off-by: Dominik Linsmayer <50681661+DoMaLi94@users.noreply.github.com>
@DoMaLi94 DoMaLi94 requested a review from yunchu as a code owner April 1, 2024 15:22
Copy link
Contributor

@samet-akcay samet-akcay left a comment

Choose a reason for hiding this comment

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

thanks for your contribution!

@samet-akcay
Copy link
Contributor

The tests are failing due to 601_mlflow_logging.ipynb. We will need to have a look at it

@DoMaLi94
Copy link
Contributor Author

DoMaLi94 commented Apr 2, 2024

The tests are failing due to 601_mlflow_logging.ipynb. We will need to have a look at it

The test fails due to the "magic command" !mlflow server, which runs infinitely. I'll take care of it.

@Youho99
Copy link
Contributor

Youho99 commented Apr 2, 2024

Hey,

J'ai créé une intégration mlflow pour yolov9
L'architecture initale entre yolov9 et anomalib est un peu similaire

Mais l'intΓ©gration mlflow n'a pas besoin d'etre Γ  proprement parlΓ© un logger car les arguments peuvet Γͺtre passΓ© par variables d'environnements.

Je vous laisse jeter un oeil pour vous en inspirer, si cela vous permet de reussir Γ  construire le logger pour Anomalib
WongKinYiu/yolov9#87

Cheers !

Signed-off-by: Dominik Linsmayer <50681661+DoMaLi94@users.noreply.github.com>
Copy link
Contributor

@djdameln djdameln left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@ashwinvaidya17 ashwinvaidya17 merged commit aa99bcf into openvinotoolkit:main May 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Requires Changes Reviewers request changes, which should be addressed by the PR maker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants