Skip to content

Conversation

johanneskoester
Copy link
Contributor

@johanneskoester johanneskoester commented Sep 12, 2025

Summary by CodeRabbit

  • Refactor

    • Pinning and asset-caching operations are now asynchronous; implementations and callers must await them.
    • Archiving terminology replaced with unified caching/pinning terminology.
  • Tests

    • Tests updated to exercise async pinning and caching flows and include a new pinning test.
    • Test environment setup simplified with explicit settings, executables, temp dirs, and prefix parameters.

Copy link
Contributor

coderabbitai bot commented Sep 12, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Converted two abstract methods to async coroutines (PinnableEnvBase.pin, CacheableEnvBase.cache_assets), updated tests to await them, replaced archive-based tests with caching/pinning, and refactored test environment construction to use explicit keyword arguments and new directories.

Changes

Cohort / File(s) Summary
Core interface: async API updates
snakemake_interface_software_deployment_plugins/__init__.py
Changed method signatures to async: PinnableEnvBase.pin() and CacheableEnvBase.cache_assets(). No other logic changes; callers must now await these coroutines.
Tests: adapt to async and new env setup
snakemake_interface_software_deployment_plugins/tests.py
Replaced ArchiveableEnvBase usage with CacheableEnvBase and added PinnableEnvBase. Updated tests to await env.cache_assets() and await env.pin() where applicable. Added test_pin. Refactored _get_env to pass explicit keyword args (settings, shell_executable, tempdir, deployment_prefix, cache_prefix, pinfile_prefix) and create temp/deploy/cache/pin directories. Added test helpers test_envspec_str, test_default_settings, get_settings, and get_settings_cls.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Tester
  participant Tests
  participant Env as Env (Cacheable / Pinnable)

  Tester->>Tests: run tests
  alt Env supports caching
    Tests->>+Env: await cache_assets()
    Env-->>-Tests: assets cached
  end
  alt Env supports pinning
    Tests->>+Env: await pin()
    Env-->>-Tests: pinned
  end
  Tests->>+Env: deploy()
  Env-->>-Tests: deployed
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


📜 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 8dfe3fb and d568d92.

📒 Files selected for processing (1)
  • snakemake_interface_software_deployment_plugins/tests.py (4 hunks)
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/adapt-tests

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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (1)
snakemake_interface_software_deployment_plugins/tests.py (1)

4-4: Fix CI: remove unused import 'tempfile'

Ruff flags this as F401. Safe to delete.

Apply:

-from copy import deepcopy
-import tempfile
+from copy import deepcopy
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 713d909 and 8dfe3fb.

📒 Files selected for processing (2)
  • snakemake_interface_software_deployment_plugins/__init__.py (2 hunks)
  • snakemake_interface_software_deployment_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:

  • snakemake_interface_software_deployment_plugins/__init__.py
  • snakemake_interface_software_deployment_plugins/tests.py
🧠 Learnings (1)
📚 Learning: 2025-09-11T11:26:53.399Z
Learnt from: johanneskoester
PR: snakemake/snakemake-interface-software-deployment-plugins#18
File: snakemake_interface_software_deployment_plugins/__init__.py:278-282
Timestamp: 2025-09-11T11:26:53.399Z
Learning: In snakemake_interface_software_deployment_plugins, the cache_path property in CacheableEnvBase intentionally returns only self._cache_prefix without appending the environment hash, as the cache is meant to be shared across environments for efficiency.

Applied to files:

  • snakemake_interface_software_deployment_plugins/tests.py
🧬 Code graph analysis (1)
snakemake_interface_software_deployment_plugins/tests.py (1)
snakemake_interface_software_deployment_plugins/__init__.py (7)
  • CacheableEnvBase (268-297)
  • DeployableEnvBase (300-360)
  • EnvBase (126-241)
  • EnvSpecBase (34-123)
  • PinnableEnvBase (244-265)
  • cache_assets (272-276)
  • pin (250-255)
🪛 GitHub Actions: CI
snakemake_interface_software_deployment_plugins/tests.py

[error] 4-4: Ruff check failed: Unused import 'tempfile' in tests.py (F401). Remove unused import. 2 fixable with '--fix'.


[error] 11-11: Ruff check failed: Unused import 'ArchiveableEnvBase' imported but unused (F401). Remove unused import. 2 fixable with '--fix'.

🔇 Additional comments (3)
snakemake_interface_software_deployment_plugins/__init__.py (2)

250-255: Async-ify pin(): API change looks good

Switching PinnableEnvBase.pin() to async aligns it with other async operations (e.g., deploy). Tests already await it. No further changes needed here.


272-276: Async-ify cache_assets(): API change looks good

Making CacheableEnvBase.cache_assets() async is consistent with get_cache_assets() and the new test usage. Good move.

snakemake_interface_software_deployment_plugins/tests.py (1)

134-142: Env construction via explicit keywords LGTM

Passing deployment_prefix, cache_prefix, and pinfile_prefix explicitly is clear and future-proof.

@johanneskoester johanneskoester merged commit 3f37e5f into main Sep 12, 2025
3 of 4 checks passed
@johanneskoester johanneskoester deleted the fix/adapt-tests branch September 12, 2025 08:27
johanneskoester pushed a commit that referenced this pull request Sep 12, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.7.1](v0.7.0...v0.7.1)
(2025-09-12)


### Bug Fixes

* adapt tests to API changes
([#19](#19))
([3f37e5f](3f37e5f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant