fix: scheduler test set()

[fgvieira]

• Use set() on tests, otherwise assert will fail if order of jobs is different. Or the scheduler could also return a set(), but not sure if it breaks backwards compatibility.
• Add an extra job that should not be scheduled to test if proper jobs are chosen.
• Add release-please token

Summary by CodeRabbit

• Tests

  • Added a dummy job and an extra dependency in the test DAG to broaden scheduler coverage.
  • Updated scheduler test to include the additional candidate when selecting jobs.
  • Relaxed ordering checks by switching from list equality to set-based assertions.
  • Minor assertion message and formatting tweaks.

• Chores

  • Updated release workflow configuration to include explicit token input and preserve secret key formatting.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a new DummyJob to the test DAG, makes it a dependency of an existing job, includes it in the scheduler candidate set for the second selection, and adjusts test assertions (one remains a single-item check; another changed from list to set equality). Minor assertion formatting tweaks only.

Changes

Cohort / File(s)	Summary
Test updates src/snakemake_interface_scheduler_plugins/tests.py	Adds a new DummyJob (input1.txt → output4.txt, cpu=1, mem_mb=10024), sets self._jobs[3] to depend on self._jobs[0], includes dag._jobs[3] in the second select_jobs candidate set, and updates assertions (one preserved as a single-item check; another changed from list equality to set equality). Minor assertion message/formatting tweaks.
CI workflow .github/workflows/release-please.yml	Adds a token input to the GoogleCloudPlatform/release-please-action step and adjusts the PyPI publish step to explicitly include the PYPI_TOKEN secret key line (formatting adjustment, no functional change).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "fix: scheduler test set()" directly and specifically refers to the primary change in this pull request. The main modifications are in the test file where assertions have been changed from list equality to set equality to prevent ordering-dependent failures. This is confirmed by the PR objectives which explicitly state that "Tests were changed to use set() when comparing scheduled jobs." The title is concise, avoids vague terminology, and clearly communicates the core fix to the scheduler tests. While the secondary workflow file changes are not mentioned in the title, this is appropriate since the title should focus on the most important change, which this does.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d530573 and d43a7ad.

📒 Files selected for processing (1)

• .github/workflows/release-please.yml (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: title-format

🔇 Additional comments (3)

.github/workflows/release-please.yml (3)

19-19: ✓ Proper use of scoped authentication token.

Adding an explicit token input with a dedicated secret follows least-privilege best practices and improves security by isolating release automation permissions from general CI permissions.

---

39-39: ✓ Correct PyPI token configuration.

Using a dedicated PYPI_TOKEN secret with the __token__ user is the correct authentication pattern for PyPI publishing.

---

1-39: ⚠ Discrepancy: File provided does not match PR objectives.

The PR objectives describe changes to scheduler tests (adding DummyJob, using set() for assertions), but the provided file is a GitHub Actions workflow configuration unrelated to these changes. The AI summary references src/snakemake_interface_scheduler_plugins/tests.py, which was not provided for review.

The workflow changes shown are correct, but they appear to be incidental CI/CD configuration updates unrelated to the core fix described in the PR.

Please confirm that all relevant files from this PR have been provided for review. Specifically, verify that src/snakemake_interface_scheduler_plugins/tests.py should be included, as it is referenced in both the PR description and AI-generated summary.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/snakemake_interface_scheduler_plugins/tests.py (1)

131-153: Consider converting results to sets in tests instead of changing the return type.

If modifying the scheduler interface's return type is not feasible (e.g., due to backward compatibility concerns), consider converting the results to sets only in the assertions:

-        assert scheduled == set([]), (
+        assert set(scheduled) == set(), (
             "Scheduler should not select jobs exceeding available resources"
         )

This approach:

• Maintains backward compatibility if existing code expects lists
• Achieves order-independent comparisons
• Requires changes only in tests, not in the interface

However, if changing the interface to return sets is the intended design, ensure that:

1. The base class or interface definition is updated with the correct return type annotation
2. All implementations are updated to return sets
3. All callers are updated to handle sets instead of lists

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ae4a3f and 0f5ae12.

📒 Files selected for processing (1)

• src/snakemake_interface_scheduler_plugins/tests.py (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

• src/snakemake_interface_scheduler_plugins/tests.py

🪛 GitHub Actions: CI

src/snakemake_interface_scheduler_plugins/tests.py

[error] 131-131: AssertionError: Scheduler should not select jobs exceeding available resources

🔇 Additional comments (1)

src/snakemake_interface_scheduler_plugins/tests.py (1)

141-141: Ensure the scheduler implementation returns sets.

These assertions will also fail if select_jobs returns a list instead of a set. While changing to sets is appropriate for order-independent comparisons (as mentioned in the PR description), the actual select_jobs method implementation must also be updated to return a set instead of a list.

If the implementation already returns a set, then the type annotations and return statements throughout the codebase should be verified for consistency.

Verify that all scheduler implementations return sets:

Also applies to: 151-151

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/snakemake_interface_scheduler_plugins/base.py (1)

48-71: Update docstring to reflect the return type change.

Line 70 of the docstring states "Otherwise, return the sequence of selected jobs," but the method now returns a set, not a sequence.

Apply this diff to fix the documentation:

         Return None to indicate an error in the selection process that shall lead to
         a fallback to the Snakemake's internal greedy scheduler.
-        Otherwise, return the sequence of selected jobs.
+        Otherwise, return the set of selected jobs.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f5ae12 and a2185df.

📒 Files selected for processing (1)

• src/snakemake_interface_scheduler_plugins/base.py (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

• src/snakemake_interface_scheduler_plugins/base.py

🧬 Code graph analysis (1)

src/snakemake_interface_scheduler_plugins/base.py (1)

src/snakemake_interface_scheduler_plugins/interfaces/jobs.py (1)

• JobSchedulerInterface (7-18)

🔇 Additional comments (1)

src/snakemake_interface_scheduler_plugins/base.py (1)

7-7: LGTM!

The addition of Set to the imports is necessary to support the return type change in the select_jobs method.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/snakemake_interface_scheduler_plugins/tests.py (3)

131-133: Fix addresses the pipeline failure correctly.

Converting scheduled to a set before comparison resolves the type mismatch and enables order-independent assertions. This approach is flexible—it works whether select_jobs returns a list, tuple, or set.

Minor: prefer set() over set([]) for empty sets—it's more idiomatic and avoids creating an intermediate list:

-        assert set(scheduled) == set([]), (
+        assert set(scheduled) == set(), (
             "Scheduler should not select jobs exceeding available resources"
         )

---

141-143: Order-independent comparison implemented correctly.

The set conversion ensures the assertion passes regardless of job ordering in the returned collection.

Consider simplifying the right-hand side for clarity:

-        assert set(scheduled) == set([dag._jobs[0]]), (
+        assert set(scheduled) == {dag._jobs[0]}, (
             "Scheduler did not select the expected job"
         )

---

153-155: Multiple job selection validated correctly with order independence.

The set conversion properly handles the case where both jobs should be selected, regardless of their order in the result.

For consistency with the other assertions, consider using set literal syntax:

-        assert set(scheduled) == set([dag._jobs[1], dag._jobs[2]]), (
+        assert set(scheduled) == {dag._jobs[1], dag._jobs[2]}, (
             "Scheduler did not select the expected jobs"
         )

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cc568a8 and 726068d.

📒 Files selected for processing (1)

• src/snakemake_interface_scheduler_plugins/tests.py (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

• src/snakemake_interface_scheduler_plugins/tests.py

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/snakemake_interface_scheduler_plugins/tests.py (2)

136-138: Apply set comparison consistently per PR objective.

The PR objective is to use set() for test comparisons to handle different job ordering. While this assertion compares against an empty result (where ordering doesn't matter), for consistency with the PR objective and line 158's approach, consider using set comparison here as well.

Apply this diff:

-        assert scheduled == [], (
+        assert set(scheduled) == set(), (
             "Scheduler should not select jobs exceeding available resources"
         )

Note: This also addresses the type mismatch mentioned in the previous review if select_jobs returns a Sequence type, since [] != set([]) in Python.

---

146-148: Apply set comparison consistently per PR objective.

The PR objective is to use set() for test comparisons to handle different job ordering. While there's only one job here, applying set comparison consistently aligns with the PR's purpose and matches the approach used at line 158.

Apply this diff:

-        assert scheduled == [dag._jobs[0]], (
+        assert set(scheduled) == {dag._jobs[0]}, (
             "Scheduler did not select the expected job"
         )

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 726068d and 3a61301.

📒 Files selected for processing (1)

• src/snakemake_interface_scheduler_plugins/tests.py (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

• src/snakemake_interface_scheduler_plugins/tests.py

🧬 Code graph analysis (1)

src/snakemake_interface_scheduler_plugins/tests.py (2)

src/snakemake_interface_scheduler_plugins/interfaces/jobs.py (3)

• input (24-26)
• output (30-32)
• SingleJobSchedulerInterface (21-49)

src/snakemake_interface_scheduler_plugins/base.py (1)

• select_jobs (41-72)

🪛 Ruff (0.14.0)

src/snakemake_interface_scheduler_plugins/tests.py

74-78: Positional argument cannot follow keyword argument

(invalid-syntax)

🔇 Additional comments (1)

src/snakemake_interface_scheduler_plugins/tests.py (1)

152-160: LGTM: Set comparison correctly applied and test logic is sound.

The set comparison at line 158 correctly implements the PR objective. The test appropriately validates that the scheduler excludes dag._jobs[3] (which requires 10024 MB but only 10000 MB is available) while selecting jobs 1 and 2 that fit within the resource constraints.