-
Notifications
You must be signed in to change notification settings - Fork 174
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
Remove the batch suffix from examples #1364
Remove the batch suffix from examples #1364
Conversation
tests/run_examples.py
Outdated
skiped_files = ['__init__.py', | ||
'spmd_utils.py', | ||
'log_reg_model_builder.py', | ||
'n_jobs.py', |
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.
but we would like to run those examples to validate that they are work
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.
but we would like to run those examples to validate that they are work
Most of these files are not the examples, but some helper functions. For this reason we shouldn't run it as examples. The only exception is log_reg_model_builder.py
. For some unknown reason this file didn't have any suffix, for this reason, it wasn't launching before at all. So we have two independent tasks here:
- Remove
_batch
suffix for more clear naming (this PR). - Investigate the problem with
log_reg_model_builder.py
and prepare a fix for this problem (some other PR). After the problem will be fixed, we can remove this file from the list of skipping examples.
This plan looks reasonable from my point of view, what is your opinion?
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.
@razdoburdin I think as an option you can move non-example, helpers scripts into utils module examples/utils
.
/intelci: run |
…b.com/razdoburdin/scikit-learn-intelex into remove_the_batch_suffix_from_examples
/intelci: run |
/intelci: run |
/intelci: run |
/intelci: run |
GPU CI job run is here. |
Should examples in daal4py/sycl also have the batch suffix removed? |
I think yes, I suggest removing it in a separate PR. |
I think it could be added to this one if we are already doing both daal4py and sklearnex examples in this one |
For reference, these are the examples that run in this job that did not run previously: daal4py/log_reg_model_builder.py Of these, it seems like the model builders should be ran. We have a separate step in recipe testing for daal4py/sycl/sklearn_sycl.py so this should be excluded from run_examples file, as well as spmd_utils since its just helper functions. Are sklearnex/ njobs, patch_sklearn, and verbose_mode intended to be ran with run_examples.py (@samir-nasibli)? |
/intelci: run |
GPU CI job run is here. |
|
daal4py/log_reg_model_builder.py - we can technically drop this if there are problems there. meanwhile lot of failures with - TypeError: create_pytest_switches() missing 1 required positional argument: 'preview' |
This should be fixed by rebase (#1418) |
@mergify rebase |
❌ Base branch update has failedGit reported the following error:
err-code: 0B313 |
/intelci: run |
GPU CI job run is here. |
daal4py/sycl/sklearn_sycl.py |
…b.com/razdoburdin/scikit-learn-intelex into remove_the_batch_suffix_from_examples
done |
In the best of my understanding, they should be launched. Multiple examples weren't launching before, due to the improper naming. |
/intelci: run |
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.
ok with changes - assuming CI would be green
Description
In continue of discussion started by @samir-nasibli in #1320 (comment) and #1360.
I prepared an update for tests/run_examples.py:
_batch
suffix in the priv version.Issue:
I found, that
log_reg_model_builder.py
return error. This file wasn't launching before. I have temporarily added it to the skipping list. We should fix this example or remove it.