-
Notifications
You must be signed in to change notification settings - Fork 322
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
Speed up SDV-Enterprise integration tests #2331
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2331 +/- ##
==========================================
- Coverage 98.58% 98.57% -0.02%
==========================================
Files 58 58
Lines 6085 6086 +1
==========================================
Hits 5999 5999
- Misses 86 87 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
sdv/datasets/demo.py
Outdated
@lru_cache() | ||
def _download_demo(modality, dataset_name, output_folder_name=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of caching here, can we make a download_test_demo fixture in our test utils and just cache that one?
tmp_path = tempfile.mktemp(dir=store_path, suffix='.yml') | ||
shutil.copyfile(config_path, tmp_path) | ||
shutil.move(tmp_path, store_path / 'sdv_logger_config.yml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is required because multiple workers can access the logger at the same time ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this function can be called simultaneously by multiple workers, so while one of them was copying config_path into store_path, another one was resetting the store_path/config_path variables.
Resolves https://github.com/datacebo/SDV-Enterprise/issues/995