Skip to content
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.

Sign up for GitHub

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

Introduce fairseq2n pytest marker #83

Closed
wants to merge 1 commit into from
Closed

Introduce fairseq2n pytest marker #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions tests/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,3 @@ def tmp_rng_seed(device: Device, seed: int = 0) -> Generator[None, None, None]:
torch.manual_seed(seed)

yield


def python_devel_only() -> bool:
"""Return ``True`` if fairseq2 is installed for Python development only."""
import fairseq2
import fairseq2n

return fairseq2.__version__ != fairseq2n.__version__
48 changes: 41 additions & 7 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,18 @@
# This source code is licensed under the BSD-style license found in the
# LICENSE file in the root directory of this source tree.

import warnings
from argparse import ArgumentTypeError
from pathlib import Path
from typing import cast

import pytest
from packaging.version import Version

import tests.common
from fairseq2.typing import Device


def parse_device_arg(value: str) -> Device:
try:
return Device(value)
except RuntimeError:
raise ArgumentTypeError(f"'{value}' is not a valid device name.")


def pytest_addoption(parser: pytest.Parser) -> None:
# fmt: off
parser.addoption(
Expand All @@ -34,6 +29,20 @@ def pytest_addoption(parser: pytest.Parser) -> None:
# fmt: on


def parse_device_arg(value: str) -> Device:
try:
return Device(value)
except RuntimeError:
raise ArgumentTypeError(f"'{value}' is not a valid device name.")


def pytest_configure(config: pytest.Config) -> None:
config.addinivalue_line(
"markers",
"fairseq2n(version): mark test to run only on the specified fairseq2n version or greater",
)


def pytest_sessionstart(session: pytest.Session) -> None:
tests.common.device = cast(Device, session.config.getoption("device"))

Expand All @@ -46,3 +55,28 @@ def pytest_ignore_collect(
return not cast(bool, config.getoption("integration"))

return False


def pytest_runtest_setup(item: pytest.Function) -> None:
marker = item.get_closest_marker(name="fairseq2n")
if marker is not None:
skip_if_fairseq2n_newer(*marker.args, **marker.kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we end up using custom marks can we have a link to documentation ? https://docs.pytest.org/en/stable/reference/reference.html#custom-marks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I just realized that I should also update the contribution guidelines and mention this annotation there.



def skip_if_fairseq2n_newer(version: str) -> None:
import fairseq2n

installed_version = Version(fairseq2n.__version__)
annotated_version = Version(version)

# fmt: off
if installed_version < annotated_version:
pytest.skip(f"The test requires fairseq2n v{annotated_version} or greater.")
elif (
installed_version.major != annotated_version.major or
installed_version.minor != annotated_version.minor
):
warnings.warn(
f"The test requires fairseq2n v{annotated_version} which is older than the current version (v{installed_version}). The marker can be safely removed."
)
# fmt: on
7 changes: 2 additions & 5 deletions tests/unit/data/data_pipeline/test_sample.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,12 @@
from fairseq2.data import DataPipeline, read_sequence
from fairseq2.data.text.text_reader import read_text
from fairseq2.utils.version import is_pt2_or_greater
from tests.common import python_devel_only, tmp_rng_seed
from tests.common import tmp_rng_seed

cpu_device = torch.device("cpu")


@pytest.mark.skipif(
python_devel_only(),
reason="New fairseq2n API in Python-only installation. Skipping till v0.2.",
)
@pytest.mark.fairseq2n("0.2a0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to have a custom pytest marks?
why not using skip_if_fairseq2n_newer as an annotation ?

def skip_if_fairseq2n_newer(version):
  ...
  if ...:
     warnings.warn(...)
  return pytest.skip(installed_version < annotated_version, reason=f"The test requires fairseq2n v{annotated_version} or greater.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH no particular reason. I just followed the instructions here: https://docs.pytest.org/en/7.1.x/example/markers.html. Looks like one advantage of using a pytest marker is that it integrates your marker with the pytest CLI (described here). It also makes it possible to use a marker in parametrized tests. Let me know if you have any objection on using a pytest marker though. I don't have a strong opinion on this.

@pytest.mark.skipif(
not is_pt2_or_greater(),
reason="Different sampling results with versions lower than PyTorch 2.0",
Expand Down