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

Record block execution time #128

Merged
merged 8 commits into from
Sep 16, 2020
Merged

Record block execution time #128

merged 8 commits into from
Sep 16, 2020

Conversation

sarahmish
Copy link
Contributor

@sarahmish sarahmish commented Jul 28, 2020

Resolve #127.

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

I made some comments in the corresponding issue suggesting an alternative implementation that I think would be more extensible and useful in the long run, but I would not mind merging this as an interim solution while we do not decide and develop the final approach, as far as the comment about the Keras dependency is addressed.

setup.py Outdated
@@ -15,6 +15,7 @@


install_requires = [
'Keras>=2.1.6,<2.4'
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be added here, since Keras is not a direct dependency of mlblocks.
Also note that current MLPrimitives master already fixed this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the test dependency to use MLPrimitives master to fix this.

Also I apologize for the multiple commits/pushes, tests were passing just fine locally as they were using different versions of dependencies. I capped the issues that arise from newer versions.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #128 into master will increase coverage by 0.67%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   78.78%   79.45%   +0.67%     
==========================================
  Files           5        5              
  Lines         707      735      +28     
==========================================
+ Hits          557      584      +27     
- Misses        150      151       +1     
Impacted Files Coverage Δ
mlblocks/mlpipeline.py 88.92% <94.87%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7337ee1...5e44993. Read the comment docs.

@csala csala merged commit dbb6477 into MLBazaar:master Sep 16, 2020
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.

Get execution time for each block
4 participants