Refactor: Reorganize run_task and unit tests into dedicated directories #1159

DeborahOlaboye · 2025-06-23T20:23:30Z

This pull request addresses issue #591 by reorganizing the test structure to improve clarity and maintainability.

Changes:

Moved all tests related to run_task into a new tests/task/ directory.
Moved unit tests to the tests/unit/ directory.
Verified all tests pass using task test to ensure functionality remains unchanged.

Closes #591

…d correct base_dir resolution in sanitize_marian_args

DeborahOlaboye · 2025-06-27T16:31:55Z

Hi @gregtatum, just wanted to see if you’ve had a chance to look at this. I'd be willing to make any changes needed. Thank you.

gregtatum

This is looking much nicer! I'm requesting changes for the following:

pathlib inconsistencies, and the parent[1] pattern being confusing.
pyproject.toml exclude list needs updating for the type suppressions.
Dependency changes need to be reverted

Plus there were a few other smaller things I commented on. This is getting close! Thanks for the work on it.

gregtatum · 2025-06-30T18:53:55Z

pipeline/eval/requirements/eval.in

@@ -1,2 +1,4 @@
 sacrebleu[ja,ko]==2.4.2
 unbabel-comet==2.2.2
+numpy==1.26.4


I don't think a refactor for file paths should need to change any dependencies here. I know our dependency situation can be brittle across different machines. We pretty much require that everything run in docker, due to things breaking outside of docker.

task docker will start up docker. If you can try on main to run that and verify that the tests run locally for you without changing any dependencies. If it's still failing within docker then please file an issue and we'll need to fix that first.

So basically these files should be unchanged:

pipeline/eval/requirements/eval.in pipeline/eval/requirements/eval.txt pipeline/translate/requirements/translate-ctranslate2.in pipeline/translate/requirements/translate-ctranslate2.txt poetry.lock pyproject.toml <- revert dependency changes, keep other changes

gregtatum · 2025-06-30T18:56:59Z

pyproject.toml

@@ -57,7 +57,7 @@ hanzidentifier = "1.2.0"
 psutil= "6.0.0"

 [tool.poetry.group.utils-docker.dependencies]
-PyICU = "2.8.1"
+PyICU = "^2.11"


Ah, it looks like someone is updating PyICU again. We'll probably want to update it, but that should be a different PR. You are welcome to open another one with this change, but it's best to keep PRs as small as possible.

gregtatum · 2025-06-30T18:57:18Z

pyproject.toml

+PyICU = "^2.11"
+
+
+[tool.poetry.group.dev.dependencies]


This probably can be reverted as well.

gregtatum · 2025-06-30T18:57:28Z

pyproject.toml

@@ -120,6 +126,7 @@ build-backend = "poetry.core.masonry.api"

 [tool.pytest.ini_options]
 testpaths = ["tests"]
+pythonpath = ["."]


This seems like a reasonable change to me.

gregtatum · 2025-06-30T18:58:48Z

pyproject.toml

@@ -120,6 +126,7 @@ build-backend = "poetry.core.masonry.api"

 [tool.pytest.ini_options]
 testpaths = ["tests"]
+pythonpath = ["."]
 markers = [
  # Run tests outside of docker:
  #   task test -- -m "not docker_amd64


I can't comment on the files themselves, but below there are:

tests/data/tests/en-ca-teacher-1.npz tests/data/tests/en-ca-vocab.spm

I think these don't need to be added to this PR, and should be removed. This should be a refactor so we'll not need more files.

gregtatum · 2025-06-30T19:03:57Z

tests/task/test_eval.py


 en_fake_translated = "\n".join([line.upper() for line in ru_sample.split("\n")])
 ru_fake_translated = "\n".join([line.upper() for line in en_sample.split("\n")])

 current_folder = os.path.dirname(os.path.abspath(__file__))
-fixtures_path = os.path.join(current_folder, "fixtures")
-root_path = os.path.abspath(os.path.join(current_folder, ".."))
+fixtures_path = (Path(__file__).resolve().parents[1] / "fixtures").as_posix()


I wouldn't mix the pathlib and os.path utilities here. I would prefer one or the other.

The simplest would be to make this work without pathlib. I generally prefer pathlib, but that does mean changing more lines of code here.

gregtatum · 2025-06-30T19:04:24Z

tests/task/test_eval.py

@@ -106,12 +107,12 @@ def run_eval_test(params) -> None:
        }

    if comet == "skipped":
-        env["COMET_SKIP"] = "1"
+        env["COMET_SKIP"] = "1"  # type: ignore


Again, the exclude paths need updating in the pyproject.toml. Rather than fixing files in a big PR, it's better to split them into smaller PRs.

gregtatum · 2025-06-30T19:05:12Z

tests/task/test_training.py


 pytestmark = [pytest.mark.docker_amd64]

 current_folder = os.path.dirname(os.path.abspath(__file__))
-fixtures_path = os.path.join(current_folder, "fixtures")
+fixtures_path = (Path(__file__).resolve().parents[1] / "fixtures").as_posix()


Same here with my previous comments on pathlib. I'll stop commenting on this, but if you can look through the rest of the PR for anything else where my feedback would apply.

gregtatum · 2025-06-30T19:05:35Z

tests/task/test_training.py

@@ -19,15 +20,15 @@


 def validate_alignments(corpus_path, vocab_src_path, vocab_trg_path):
-    sp_src = spm.SentencePieceProcessor(model_file=vocab_src_path)
-    sp_trg = spm.SentencePieceProcessor(model_file=vocab_trg_path)
+    sp_src = spm.SentencePieceProcessor(model_file=vocab_src_path)  # type: ignore


Same here, I'll stop commenting on the type suppressions, but the exclude list should be fixed instead.

gregtatum · 2025-06-30T19:08:07Z

tests/unit/test_tracking_cli.py



 @patch(
    "translations_parser.cli.taskcluster.get_args",
    return_value=argparse.Namespace(
-        input_file=Path(__file__).parent / "data" / "taskcluster.log",
+        input_file=Path(__file__).resolve().parents[1] / "data" / "taskcluster.log",


To expand on my earlier comments around pathlib, I find this style to be really confusing where you are indexing into the array of parents.

I would prefer something simpler like:

Path(__file__).parent / "../data/taskcluster.log"

gregtatum · 2025-06-30T19:10:57Z

Oh and when you need me to re-review this, please hit the little circle with arrows next to my name, and it will show up on my review queue. If you are pushing up without changes ready for review, just make sure I'm not flagged for review.

Here is where the circle arrow thing is:

DeborahOlaboye · 2025-07-02T12:20:47Z

Hi @gregtatum,

I've made the requested corrections and pushed the updates. However, two tests are currently failing:

tests/unit/test_tracking_cli.py::test_experiments_marian_1_10

tests/unit/test_tracking_cli.py::test_experiments_marian_1_12

The logs indicate assertion errors, but the exact details of the failures are not fully exposed in the test summary.

From the logs, it seems the failure might be linked to how experiments Marian versions 1.10 and 1.12 are parsed, possibly affected by how the changes interact with log parsing or configuration file access in the Taskcluster context.

I'm currently investigating this further, but if you can confirm whether this failure is expected due to a known issue with these versions, I’d appreciate your input.

Thank you once again.

refactor tests structure

8e7511b

DeborahOlaboye requested review from a team as code owners June 23, 2025 20:23

DeborahOlaboye added 11 commits June 24, 2025 04:26

fixed an issue around tests failing

190bf4d

correct fixtures path to use tests/fixtures for spm_train

317e0a0

correct path to fixtures directory for spm_train

f976262

correct path to fixtures

96a519d

correct path to fixtures

2262675

correct path to fixtures

8d26b08

correct path to fixtures

aa0a2db

correct path to fixtures

27a28bf

correct path to fixtures

84da404

correct path to fixtures

fa774ae

correct path to fixtures

dfdb666

DeborahOlaboye marked this pull request as draft June 25, 2025 06:00

correct path to fixtures

083016d

DeborahOlaboye force-pushed the main branch from 6c2dea6 to 083016d Compare June 25, 2025 21:43

DeborahOlaboye added 12 commits June 25, 2025 23:02

fix: ignore Pyright type error

1dae48d

ensure zstandard is included in task venv by updating requirements an…

fe7f75f

…d correct base_dir resolution in sanitize_marian_args

correct path for task graph

fedee8e

fix issues around specific tests failing

e1f35bf

Update compiled requirements with hashes

a75df6b

Update compiled requirements with hashes

290a390

Update compiled requirements with hashes

c48a38b

Update compiled requirements with hashes

189e1d9

Update compiled requirements with hashes

bc791cb

Update compiled requirements with hashes

e8315a4

Fix quantized Marian decoder config path in test setup

7127cf7

Fix quantized Marian decoder config path in test setup

b12277f

DeborahOlaboye marked this pull request as ready for review June 26, 2025 18:11

DeborahOlaboye added 5 commits June 26, 2025 21:09

Fix directory path for test_ctranslate2

0378fd2

refactor specific tests structure

6759416

format file

d67b6fb

correct path

4e97b6b

correct path

da0efe2

DeborahOlaboye changed the title ~~Refactor: Migrate all the run_task tests into a separate folder (#591)~~ Refactor: Migrate all the run_task tests into a separate folder Jun 27, 2025

DeborahOlaboye changed the title ~~Refactor: Migrate all the run_task tests into a separate folder~~ Refactor: Reorganize run_task and unit tests into dedicated directories Jun 27, 2025

gregtatum requested changes Jun 30, 2025

View reviewed changes

DeborahOlaboye added 6 commits July 1, 2025 22:37

Update logic as per review suggestions

721d861

fix import issues

c20f901

correct path for test_tracking_cli

4b7457a

correct directory path

510c2a3

correct directory path

73f43a6

correct directory path

9e67e37

DeborahOlaboye requested a review from gregtatum July 2, 2025 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor: Reorganize run_task and unit tests into dedicated directories #1159

Refactor: Reorganize run_task and unit tests into dedicated directories #1159

Uh oh!

DeborahOlaboye commented Jun 23, 2025 •

edited

Loading

Uh oh!

DeborahOlaboye commented Jun 27, 2025

Uh oh!

gregtatum left a comment

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum Jun 30, 2025

Uh oh!

gregtatum commented Jun 30, 2025

Uh oh!

DeborahOlaboye commented Jul 2, 2025

Uh oh!

Uh oh!

Refactor: Reorganize run_task and unit tests into dedicated directories #1159

Are you sure you want to change the base?

Refactor: Reorganize run_task and unit tests into dedicated directories #1159

Uh oh!

Conversation

DeborahOlaboye commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Uh oh!

DeborahOlaboye commented Jun 27, 2025

Uh oh!

gregtatum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregtatum commented Jun 30, 2025

Uh oh!

DeborahOlaboye commented Jul 2, 2025

Uh oh!

Uh oh!

DeborahOlaboye commented Jun 23, 2025 •

edited

Loading