chore(bigframes): add specific build script for doctest to restrict execution#16711
chore(bigframes): add specific build script for doctest to restrict execution#16711chelsea-lin merged 27 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new presubmit configuration and a bash script, run_doctest.sh, to automate doctests for the bigframes package. The script detects changes using git diff and executes nox sessions accordingly. The review feedback identifies several critical improvements for the bash script: correcting the nox command syntax to handle multiple sessions properly, adding necessary quoting to handle file paths with spaces, and implementing checks for Kokoro environment variables to prevent syntax errors in non-pull request builds.
|
Switching to draft until checks are green |
parthea
dismissed
their stale review
I just noticed that the test is skipped in the output
nox > Running session doctest
nox > Creating virtual environment (virtualenv) using python3.12 in .nox/doctest
nox > Session doctest skipped: Temporary skip to enable a PR merge. Remove skip as part of closing https://github.com/googleapis/google-cloud-python/issues/16489.
cleanup
chalmerlowe
changed the title
…in run_doctest.sh
…xpose permission errors
| >>> srs = series.Series([{"version": 1, "project": "pandas"}, {"version": 2, "project": "numpy"},]) | ||
| >>> df = srs.struct.explode() | ||
| >>> df = df[["project", "version"]] # set the column order to ensure stable output for doctest |
There was a problem hiding this comment.
The non-deterministic nature of a number of tests and doctests was really brutal.
This change helps avoid test failures in non-deterministic result generation.
There was a problem hiding this comment.
It should be a valid bug and assigned to myself b/506201379.
| {"version": 1, "project": "pandas"}, | ||
| {"version": 2, "project": "numpy"}, | ||
| {"project": "pandas", "version": 1}, | ||
| {"project": "numpy", "version": 2}, |
There was a problem hiding this comment.
The changes in this file help avoid test failures in non-deterministic result generation.
There was a problem hiding this comment.
Same here. It should be fixed by b/506201379
| if bf_div_result.dtype == pd.Int64Dtype(): | ||
| bigframes.testing.utils.assert_series_equal( | ||
| pd_div_result, bf_div_result.to_pandas() | ||
| pd_div_result, bf_div_result.to_pandas(), check_dtype=False |
There was a problem hiding this comment.
The changes in this file helps avoid test failures due to type checking.
There was a problem hiding this comment.
Is this a recent regression or mypy failure? Should we add a TODO with a link to a bug to follow up on it?
chalmerlowe
changed the title
chelsea-lin
left a comment
chelsea-lin
left a comment
There was a problem hiding this comment.
The doctest captured some regressions (b/506201379) but can be fixed after this change in.
3 participants
Warning
This is a work in progress. Do not review until invited.
Creates a build script:
packages/bigframes/scripts/run_doctest.sh. This script checks ifbigframeshas any changes (or if it's acontinuousbuild) before runnin' thecleanupdoctestsessions. This will prevent it from running when other packages are modified and failing the build.Updates the Kokoro config:
.kokoro/presubmit/presubmit-doctest-bigframes.cfgto setTRAMPOLINE_BUILD_FILEto point to the new script.