From 6a2dab0437a72d798a307da488ca1578ac8b96ca Mon Sep 17 00:00:00 2001 From: Matthew Roeschke Date: Mon, 6 Jun 2022 21:02:51 -0700 Subject: [PATCH 1/2] DOC: Move all relevant wiki items to docs --- .../development/contributing_codebase.rst | 76 ++++++++++++++++--- doc/source/development/maintaining.rst | 76 ++++++++++++++++++- doc/source/development/roadmap.rst | 46 ++++++++++- pandas/_testing/__init__.py | 1 - pandas/_testing/_io.py | 3 - 5 files changed, 184 insertions(+), 18 deletions(-) diff --git a/doc/source/development/contributing_codebase.rst b/doc/source/development/contributing_codebase.rst index 3437ddfbffdcf..6512a6d6eee12 100644 --- a/doc/source/development/contributing_codebase.rst +++ b/doc/source/development/contributing_codebase.rst @@ -304,8 +304,8 @@ This is an example of a green build. .. _contributing.tdd: -Test-driven development/code writing ------------------------------------- +Test-driven development +----------------------- pandas is serious about testing and strongly encourages contributors to embrace `test-driven development (TDD) `_. @@ -337,10 +337,11 @@ pandas existing test structure is *mostly* class-based, meaning that you will ty .. code-block:: python - class TestReallyCoolFeature: - pass + class TestReallyCoolFeature: + def test_cool_feature_aspect(self): + pass -Going forward, we are moving to a more *functional* style using the `pytest `__ framework, which offers a richer testing +We prefer a more *functional* style using the `pytest `__ framework, which offers a richer testing framework that will facilitate testing and developing. Thus, instead of writing test classes, we will write test functions like this: .. code-block:: python @@ -348,8 +349,8 @@ framework that will facilitate testing and developing. Thus, instead of writing def test_really_cool_feature(): pass -Preferred idioms -^^^^^^^^^^^^^^^^ +Preferred ``pytest`` idioms +^^^^^^^^^^^^^^^^^^^^^^^^^^^ * Functional tests named ``def test_*`` and *only* take arguments that are either fixtures or parameters. * Use a bare ``assert`` for testing scalars and truth-testing @@ -387,9 +388,66 @@ xfail is not to be used for tests involving failure due to invalid user argument For these tests, we need to verify the correct exception type and error message is being raised, using ``pytest.raises`` instead. -If your test requires working with files or -network connectivity, there is more information on the :wiki:`Testing` of the wiki. +Testing an exception +^^^^^^^^^^^^^^^^^^^^ + +Use `pytest.raises `_ as a context manager +with the specific exception subclass (i.e. never use :py:class:`Exception`) and the exception message in ``match``. + +.. code-block:: python + + with assert_produces_warning(DeprecationWarning): + pd.deprecated_function() + +If a warning should specifically not happen in a block of code, pass ``False`` into the context manager. + +.. code-block:: python + + with assert_produces_warning(False): + pd.no_warning_function() + +Testing a warning +^^^^^^^^^^^^^^^^^ + +Use ``tm.assert_produces_warning`` as a context manager to check that a block of code raises a warning. + +.. code-block:: python + + with pytest.raises(ValueError, match="an error"): + raise ValueError("an error") + +Testing involving files +^^^^^^^^^^^^^^^^^^^^^^^ + +The ``tm.ensure_clean`` context manager creates a temporary file for testing, +with a generated filename (or your filename if provided), that is automatically +deleted when the context block is exited. + +.. code-block:: python + + with tm.ensure_clean('my_file_path') as path: + # do something with the path + +Testing involving network connectivity +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +It is highly discouraged to add a test that connects to the internet due to flakiness of network connections and +lack of ownership of the server that is being connected to. If network connectivity is absolutely required, use the +``tm.network`` decorator. + +.. code-block:: python + + @tm.network # noqa + def test_network(): + result = package.call_to_internet() + +If the test requires data from a specific website, specify ``check_before_test=True`` and the site in the decorator. + +.. code-block:: python + @tm.network("https://www.somespecificsite.com", check_before_test=True) + def test_network(): + result = pd.read_html("https://www.somespecificsite.com") Example ^^^^^^^ diff --git a/doc/source/development/maintaining.rst b/doc/source/development/maintaining.rst index a8521039c5427..10718b34bff7d 100644 --- a/doc/source/development/maintaining.rst +++ b/doc/source/development/maintaining.rst @@ -138,7 +138,7 @@ Reviewing pull requests ----------------------- Anybody can review a pull request: regular contributors, triagers, or core-team -members. But only core-team members can merge pull requets when they're ready. +members. But only core-team members can merge pull requests when they're ready. Here are some things to check when reviewing a pull request. @@ -151,16 +151,37 @@ Here are some things to check when reviewing a pull request. for regression fixes and small bug fixes, the next minor milestone otherwise) * Changes should comply with our :ref:`policies.version`. + +.. _maintaining.backporting: + Backporting ----------- -In the case you want to apply changes to a stable branch from a newer branch then you -can comment:: +pandas supports point releases (e.g. ``1.4.3``) that aim to: + +1. Fix bugs in new features introduced since the first minor version release. + + * If a new feature was added in ``1.4`` and contains a bug, a fix can be applied in ``1.4.3`` + +2. Fix bugs that used to work in a few minor releases prior. + + * If a feature worked in ``1.2`` and stopped working since ``1.3``, a fix can be applied in ``1.4.3``. + +Since pandas minor releases are based on Github branches (e.g. point release of ``1.4`` are based off the ``1.4.x`` branch), +"backporting" means merging a pull request fix to the ``main`` branch and correct minor branch associated with the next point release. + +By default, if a pull request is assigned to the next point release milestone within the Github interface, +the backporting process should happen automatically by the ``@meeseeksdev`` bot once the pull request is merged. +A new pull request will be made backporting the pull request to the correct version branch. +Sometimes due to merge conflicts, a manual pull request will need to be made addressing the code conflict. + +If the bot does not automatically start the backporting process, you can also write a Github comment in the merged pull request +to trigger the backport:: @meeseeksdev backport version-branch This will trigger a workflow which will backport a given change to a branch -(e.g. @meeseeksdev backport 1.2.x) +(e.g. @meeseeksdev backport 1.4.x) Cleaning up old issues ---------------------- @@ -204,6 +225,18 @@ The full process is outlined in our `governance documents`_. In summary, we're happy to give triage permissions to anyone who shows interest by being helpful on the issue tracker. +The required steps for adding a maintainer are: + +1. Contact the contributor and ask their interest to join. +2. Add the contributor to the appropriate `Github Team `_ if accepted the invitation. + + * ``pandas-core`` is for core team members + * ``pandas-triage`` is for pandas triage members + +3. Add the contributor to the pandas Google group. +4. Create a pull request to add the contributor's Github handle to ``pandas-dev/pandas/web/pandas/config.yml``. +5. Create a pull request to add the contributor's name/Github handle to the `governance document `_. + The current list of core-team members is at https://github.com/pandas-dev/pandas-governance/blob/master/people.md @@ -236,5 +269,40 @@ a milestone before tagging, you can request the bot to backport it with: @Meeseeksdev backport +.. _maintaining.asv-machine: + +Benchmark machine +----------------- + +The team currently owns dedicated hardware for hosting a website for pandas' ASV performance benchmark. The results +are published to http://pandas.pydata.org/speed/pandas/ + +Configuration +````````````` + +The machine can be configured with the `Ansible `_ playbook in https://github.com/tomaugspurger/asv-runner. + +Publishing +`````````` + +The results are published to another Github repository, https://github.com/tomaugspurger/asv-collection. +Finally, we have a cron job on our docs server to pull from https://github.com/tomaugspurger/asv-collection, to serve them from ``/speed``. +Ask Tom or Joris for access to the webserver. + +Debugging +````````` + +The benchmarks are scheduled by Airflow. It has a dashboard for viewing and debugging the results. You'll need to setup an SSH tunnel to view them + + ssh -L 8080:localhost:8080 pandas@panda.likescandy.com + + +.. _maintaining.release: + +Release process +--------------- + +The process for releasing a new version of pandas can be found at https://github.com/pandas-dev/pandas-release + .. _governance documents: https://github.com/pandas-dev/pandas-governance .. _list of permissions: https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-roles-for-an-organization diff --git a/doc/source/development/roadmap.rst b/doc/source/development/roadmap.rst index ccdb4f1fafae4..3d6861cfb3f0a 100644 --- a/doc/source/development/roadmap.rst +++ b/doc/source/development/roadmap.rst @@ -128,7 +128,51 @@ We propose that it should only work with positional indexing, and the translatio to positions should be entirely done at a higher level. Indexing is a complicated API with many subtleties. This refactor will require care -and attention. More details are discussed at :wiki:`(Tentative)-rules-for-restructuring-indexing-code` +and attention. The following principles should inspire refactoring of indexing code and +should result on cleaner, simpler, and more performant code. + +1. **Label indexing must never involve looking in an axis twice for the same label(s).** +This implies that any validation step must either: + + * limit validation to general features (e.g. dtype/structure of the key/index), or + * reuse the result for the actual indexing. + +2. **Indexers must never rely on an explicit call to other indexers.** +For instance, it is OK to have some internal method of ``.loc`` call some +internal method of ``__getitem__`` (or of their common base class), +but never in the code flow of ``.loc`` should ``the_obj[something]`` appear. + +3. **Execution of positional indexing must never involve labels** (as currently, sadly, happens). +That is, the code flow of a getter call (or a setter call in which the right hand side is non-indexed) +to ``.iloc`` should never involve the axes of the object in any way. + +4. **Indexing must never involve accessing/modifying values** (i.e., act on ``._data`` or ``.values``) **more than once.** +The following steps must hence be clearly decoupled: + + * find positions we need to access/modify on each axis + * (if we are accessing) derive the type of object we need to return (dimensionality) + * actually access/modify the values + * (if we are accessing) construct the return object + +5. As a corollary to the decoupling between 4.i and 4.iii, **any code which deals on how data is stored** +(including any combination of handling multiple dtypes, and sparse storage, categoricals, third-party types) +**must be independent from code that deals with identifying affected rows/columns**, +and take place only once step 4.i is completed. + + * In particular, such code should most probably not live in ``pandas/core/indexing.py`` + * ... and must not depend in any way on the type(s) of axes (e.g. no ``MultiIndex`` special cases) + +6. As a corollary to point 1.i, **``Index`` (sub)classes must provide separate methods for any desired validity check of label(s) which does not involve actual lookup**, +on the one side, and for any required conversion/adaptation/lookup of label(s), on the other. + +7. **Use of trial and error should be limited**, and anyway restricted to catch only exceptions +which are actually expected (typically ``KeyError``). + + * In particular, code should never (intentionally) raise new exceptions in the ``except`` portion of a ``try... exception`` + +8. **Any code portion which is not specific to setters and getters must be shared**, +and when small differences in behavior are expected (e.g. setting with ``.loc`` raises for +missing labels, getting still doesn't), they can be managed with a specific parameter. Numba-accelerated operations ---------------------------- diff --git a/pandas/_testing/__init__.py b/pandas/_testing/__init__.py index 53e003e2ed7dd..bedd7adeb45a2 100644 --- a/pandas/_testing/__init__.py +++ b/pandas/_testing/__init__.py @@ -54,7 +54,6 @@ round_trip_localpath, round_trip_pathlib, round_trip_pickle, - with_connectivity_check, write_to_compressed, ) from pandas._testing._random import ( # noqa:F401 diff --git a/pandas/_testing/_io.py b/pandas/_testing/_io.py index 1ef65f761c3f6..46f1545a67fab 100644 --- a/pandas/_testing/_io.py +++ b/pandas/_testing/_io.py @@ -250,9 +250,6 @@ def wrapper(*args, **kwargs): return wrapper -with_connectivity_check = network - - def can_connect(url, error_classes=None): """ Try to connect to the given url. True if succeeds, False if OSError From 2e2b06fc110d9a7c5ea013d2b424934d101d634d Mon Sep 17 00:00:00 2001 From: Matthew Roeschke Date: Tue, 7 Jun 2022 17:50:51 -0700 Subject: [PATCH 2/2] Address review --- .../development/contributing_codebase.rst | 18 +++++++++--------- doc/source/development/maintaining.rst | 8 ++++---- doc/source/development/roadmap.rst | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/doc/source/development/contributing_codebase.rst b/doc/source/development/contributing_codebase.rst index 6512a6d6eee12..81cd69aa384a4 100644 --- a/doc/source/development/contributing_codebase.rst +++ b/doc/source/development/contributing_codebase.rst @@ -388,28 +388,28 @@ xfail is not to be used for tests involving failure due to invalid user argument For these tests, we need to verify the correct exception type and error message is being raised, using ``pytest.raises`` instead. -Testing an exception -^^^^^^^^^^^^^^^^^^^^ +Testing a warning +^^^^^^^^^^^^^^^^^ -Use `pytest.raises `_ as a context manager -with the specific exception subclass (i.e. never use :py:class:`Exception`) and the exception message in ``match``. +Use ``tm.assert_produces_warning`` as a context manager to check that a block of code raises a warning. .. code-block:: python - with assert_produces_warning(DeprecationWarning): + with tm.assert_produces_warning(DeprecationWarning): pd.deprecated_function() If a warning should specifically not happen in a block of code, pass ``False`` into the context manager. .. code-block:: python - with assert_produces_warning(False): + with tm.assert_produces_warning(False): pd.no_warning_function() -Testing a warning -^^^^^^^^^^^^^^^^^ +Testing an exception +^^^^^^^^^^^^^^^^^^^^ -Use ``tm.assert_produces_warning`` as a context manager to check that a block of code raises a warning. +Use `pytest.raises `_ as a context manager +with the specific exception subclass (i.e. never use :py:class:`Exception`) and the exception message in ``match``. .. code-block:: python diff --git a/doc/source/development/maintaining.rst b/doc/source/development/maintaining.rst index 10718b34bff7d..1bff2eccd3d27 100644 --- a/doc/source/development/maintaining.rst +++ b/doc/source/development/maintaining.rst @@ -159,13 +159,13 @@ Backporting pandas supports point releases (e.g. ``1.4.3``) that aim to: -1. Fix bugs in new features introduced since the first minor version release. +1. Fix bugs in new features introduced in the first minor version release. - * If a new feature was added in ``1.4`` and contains a bug, a fix can be applied in ``1.4.3`` + * e.g. If a new feature was added in ``1.4`` and contains a bug, a fix can be applied in ``1.4.3`` -2. Fix bugs that used to work in a few minor releases prior. +2. Fix bugs that used to work in a few minor releases prior. There should be agreement between core team members that a backport is appropriate. - * If a feature worked in ``1.2`` and stopped working since ``1.3``, a fix can be applied in ``1.4.3``. + * e.g. If a feature worked in ``1.2`` and stopped working since ``1.3``, a fix can be applied in ``1.4.3``. Since pandas minor releases are based on Github branches (e.g. point release of ``1.4`` are based off the ``1.4.x`` branch), "backporting" means merging a pull request fix to the ``main`` branch and correct minor branch associated with the next point release. diff --git a/doc/source/development/roadmap.rst b/doc/source/development/roadmap.rst index 3d6861cfb3f0a..f935c27d9917d 100644 --- a/doc/source/development/roadmap.rst +++ b/doc/source/development/roadmap.rst @@ -171,8 +171,8 @@ which are actually expected (typically ``KeyError``). * In particular, code should never (intentionally) raise new exceptions in the ``except`` portion of a ``try... exception`` 8. **Any code portion which is not specific to setters and getters must be shared**, -and when small differences in behavior are expected (e.g. setting with ``.loc`` raises for -missing labels, getting still doesn't), they can be managed with a specific parameter. +and when small differences in behavior are expected (e.g. getting with ``.loc`` raises for +missing labels, setting still doesn't), they can be managed with a specific parameter. Numba-accelerated operations ----------------------------