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

mlflow is not required if not being used #441

Closed
wants to merge 2 commits into from
Closed

Conversation

williamFalcon
Copy link
Contributor

remove requirement to import mlflow

@williamFalcon
Copy link
Contributor Author

williamFalcon commented Oct 29, 2019

@neggert users shouldn't have to install mlflow or comet if they're not using it.
No need to raise exception, it'll just crash and users will know

@neggert
Copy link
Contributor

neggert commented Oct 29, 2019

Yeah, was just about to send a PR when I saw this. It was originally this way, but got messed up in #395. I have a different fix that I like better because it still throws an error if you explicitly try to import e.g. CometLogger without Comet installed.

@@ -3,8 +3,8 @@

try:
import mlflow
except ImportError:
raise ImportError('Missing mlflow package.')
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

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

you do not need the e, PEP8 will complain that you define a variable which is not used

@Borda
Copy link
Member

Borda commented Oct 30, 2019

@williamFalcon @neggert I do not agree with ignoring the import error. when you try to import mlflow_logger.py directly and you do not have mlflow you should get an error message... The ignore is done in logging.__init__.py where you say the mlflow or any other is not needed.

@neggert
Copy link
Contributor

neggert commented Oct 30, 2019

@Borda I think #442 does what you're asking for.

@Borda
Copy link
Member

Borda commented Oct 30, 2019

@Borda I think #442 does what you're asking for.

That one is fine, but with this PR you change the meaning of ImportError completely...
It is like you go to sweet shop with optional desk with ice-screen, you want ice -cream, go to the desk and it is empty without any information... keeping the raise Exception gives you info that there is no ice-cream if you come to the deck but it does not border you if you come to sweet shop and you do not case about ice-cream...

@williamFalcon williamFalcon deleted the mlflow branch October 31, 2019 20:41
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.

3 participants