[MODIN]: Add time results verifier for getting started notebook#871
Merged
Conversation
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
dchigarev
commented
Feb 21, 2022
| " try:\n", | ||
| " from modin.utils import get_current_execution\n", | ||
| "\n", | ||
| " execution = get_current_execution()\n", |
Contributor
Author
There was a problem hiding this comment.
The comment according get_current_execution was applied
dchigarev
commented
Feb 21, 2022
| "pip install runipy # Installing 'runipy' for extended abilities to execute the notebook", | ||
| "runipy IntelModin_GettingStarted.ipynb" | ||
| "runipy IntelModin_GettingStarted.ipynb # Test 'Modin is faster than Pandas' case", | ||
| "MODIN_CPUS=1 runipy IntelModin_GettingStarted.ipynb # Test 'Modin is slower than Pandas' case" |
Contributor
Author
There was a problem hiding this comment.
allowing for Modin to utilize only a single core to syntactically slow it down
YarShev
reviewed
Feb 21, 2022
| " execution = (\n", | ||
| " \"Can't deduce the current execution, your Modin version is too old!\"\n", | ||
| " )\n", | ||
| " print(f\"\\tExecution: {execution}\")\n", |
Contributor
There was a problem hiding this comment.
We print above Current execution is:. Should we duplicate this?
Contributor
Author
There was a problem hiding this comment.
Changed "Current execution is:" to the "Current configuration ..." above as it seems more suitable and doesn't cause such duplication
Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Contributor
|
Can we merge this PR..thanks |
praveenkk123
approved these changes
Mar 1, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Existing Sample Changes
Description
[NOTE: This PR is a copy of the accidentally closed #843 (can't be reopened)]
The notebook is supposed to show the performance improvement after switching from stock Pandas to Modin, it compares the execution times of Pandas and Modin in various scenarios. The previous version of the notebook didn't imply that the Modin could be slower than Pandas anyhow and so confused users if such happened.
The changes in this PR introduce a time verifier, that compares Modin and Pandas execution times and prints a note if Modin appears to be slower with the possible reasons for it (poor CPU performance, old Modin version, silly environment variables).
Fixes Jira MODIN6-135.
External Dependencies
The changes do not add any new dependencies.
Type of change
How Has This Been Tested?
To test the changes locally:
sample.jsoninto a single bash script.script
Would also like to get a review from Modin folks: @Garra1980 @YarShev @vnlitvinov