-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CI run coverage on multiple builds #40394
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
Conversation
c3f5dfe
to
4233bbd
Compare
77e8433
to
7f5052e
Compare
coeffs.resize(int((coeffs.size + 1) / 2), 2) | ||
coeffs = np.resize(coeffs, (int((coeffs.size + 1) / 2), 2)) |
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 was throwing an error during CI:
____________________ TestDataFramePlots.test_andrews_curves ____________________
[gw0] linux -- Python 3.8.8 /usr/share/miniconda/envs/pandas-dev/bin/python
self = <pandas.tests.plotting.test_misc.TestDataFramePlots object at 0x7f3ae76442b0>
iris = SepalLength SepalWidth PetalLength PetalWidth Name
0 5.1 3.5 1.4 ... 2.3 Iris-virginica
149 5.9 3.0 5.1 1.8 Iris-virginica
[150 rows x 5 columns]
def test_andrews_curves(self, iris):
from matplotlib import cm
from pandas.plotting import andrews_curves
df = iris
# Ensure no UserWarning when making plot
with tm.assert_produces_warning(None):
> _check_plot_works(andrews_curves, frame=df, class_column="Name")
pandas/tests/plotting/test_misc.py:158:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pandas/tests/plotting/common.py:625: in _check_plot_works
raise err
pandas/tests/plotting/common.py:618: in _check_plot_works
for ret in gen_plots(f, fig, **kwargs):
pandas/tests/plotting/common.py:645: in _gen_two_subplots
yield f(**kwargs)
pandas/plotting/_misc.py:271: in andrews_curves
return plot_backend.andrews_curves(
pandas/plotting/_matplotlib/misc.py:282: in andrews_curves
y = f(t)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
t = array([-3.14159265, -3.11001886, -3.07844506, -3.04687127, -3.01529747,
-2.98372368, -2.95214988, -2.92057608, ...900229, 2.92057608, 2.95214988, 2.98372368,
3.01529747, 3.04687127, 3.07844506, 3.11001886, 3.14159265])
def f(t):
x1 = amplitudes[0]
result = x1 / np.sqrt(2.0)
# Take the rest of the coefficients and resize them
# appropriately. Take a copy of amplitudes as otherwise numpy
# deletes the element from amplitudes itself.
coeffs = np.delete(np.copy(amplitudes), 0)
> coeffs.resize(int((coeffs.size + 1) / 2), 2)
E ValueError: cannot resize an array that references or is referenced
E by another array in this way.
E Use the np.resize function or refcheck=False
pandas/plotting/_matplotlib/misc.py:249: ValueError
notify: | ||
after_n_builds: 10 |
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 is so that codecov doesn't report on insufficient coverage prematurely
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.
is this related to why codecov often gives an Access Denied?
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.
Without this, then Codecov will report coverage as soon as one job finishes. But if that job isn't comprehensive, then it'll show e.g. "coverage 30% less than target of 80%, failed". Like this, it'll wait for multiple jobs to be done and will merge their reports
- script: | | ||
if [ "$(uname)" == "Linux" ]; then | ||
sudo apt-get update | ||
sudo apt-get install -y libc6-dev-i386 $EXTRA_APT | ||
fi | ||
displayName: 'Install extra packages' | ||
|
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.
these linux ones are all moved to .github/workflows/posix.yml
- template: ci/azure/posix.yml | ||
parameters: | ||
name: Linux | ||
vmImage: ubuntu-16.04 |
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.
moved to .github/workflows/posix.yml
- uses: conda-incubator/setup-miniconda@v2 | ||
with: | ||
activate-environment: pandas-dev | ||
channel-priority: flexible |
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.
I couldn't get the environment to resolve with strict
@jreback Would there be a capacity/credit issue if we run all Linux builds on GitHub Actions? |
no we can run on github actions w/o restrictions |
this looks pretty good. cc @jbrockmendel as i know you look at coverage lot. cc @datapythonista if any comments here. |
No objections. The CI config is pretty mysterious to me. |
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.
Looks nice. Do we have enough builds in actions to not slow the CI results with this?
.github/workflows/posix.yml
Outdated
strategy: | ||
matrix: | ||
settings: [ | ||
[ci/deps/actions-37-minimum_versions.yaml, "not slow and not network and not clipboard", "", "", "", "", ""], |
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.
I think you can define this as before, something like:
minumum_versions:
- ENV_FILE: ci/deps/actions-37-minimum_versions.yaml
- PATTERN: "not slow and not network and not clipboard"
...
Do you find this better, or couldn't you implement it this way?
Also, very minor, but instead of repeating ci/deps/
or ci/deps/actions-{}.yaml
you could move it below, when you set ENV_FILE
.
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.
Do you find this better, or couldn't you implement it this way?
Are you sure this can be done with GitHubActions without repeating all the steps? Do you have an example? (I couldn't find one, but I haven't done that much GitHubActions)
Also, very minor, but instead of repeating ci/deps/ or ci/deps/actions-{}.yaml you could move it below, when you set ENV_FILE.
Good point, thanks!
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.
Are you sure this can be done with GitHubActions without repeating all the steps? Do you have an example? (I couldn't find one, but I haven't done that much GitHubActions)
I think the best way is to use include
. You've got an example in the docs that should be similar:
do you know where this info is? |
I couldn't find information online, not sure why. But checking the messages I sent to Azure and GitHub, Azure gives 10 parallel workers by default, but we had 30, and GitHub actions gives 20, and I asked them if they could increase it, but afaik that never happened. I think we balanced things so both azure and actions had the same load, with the 30+20. If this is still the more or less the case, and we move lots of builds from azure to actions, feels like the 20 on actions will be a bottleneck, and we won't use much the 30 in azure. But not sure if things changed since I had all these discussions. |
thanks @MarcoGorelli very nice! @pandas-dev/pandas-core so this moves the linux builds to github actions, if anything looks awry pls open an issue |
Moving the posix builds to GitHubActions + uploading coverage on every build, as they're merged by default by Codecov
Trying to get this in so that the codecov check in #40078 will pass