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

Fix/avoid sagify module import clash #106

Merged
merged 2 commits into from Jan 12, 2020

Conversation

pm3310
Copy link
Contributor

@pm3310 pm3310 commented Jan 11, 2020

Why?
sagify init automatically creates a module called sagify. However, we have specific methods from sagify library such as from sagify.api.hyperparameter_tuning import log_metric. Python interpreter gets confused if it tries to load from sagify.api.hyperparameter_tuning import log_metric. It think it should search under the automatically created module sagify, then, it cannot find and raises an error. This PR fixes this issue.

@pm3310 pm3310 requested a review from ilazakis January 11, 2020 13:28
Copy link
Contributor

@ilazakis ilazakis left a comment

Choose a reason for hiding this comment

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

LGTM, one question about the boto3 dependency @pm3310

'.gitkeep'
]
},
install_requires=[
'boto3==1.9.220',
'boto3',
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason for removing the pinned version of boto3 in particular?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ilazakis , users have to use awscli that depends on boto3. Each end user has a different version of awscli and by specifying a specific boto3 version may cause conflict.

@pm3310 pm3310 merged commit cb4a836 into develop Jan 12, 2020
@pm3310 pm3310 deleted the fix/avoid-sagify-module-import-clash branch January 12, 2020 10:43
@@ -1,59 +1,59 @@
from collections import namedtuple
Copy link
Contributor

Choose a reason for hiding this comment

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

@pm3310 Missed this when reviewing. Did these tests need to be disabled? Can we re-enable them? Also, looks like the 0.20.0 merge failed on Python3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilazakis good catch man! My bad :-( I'll create a new PR for this one

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