feat: use uv as package manager, ruff as linter, add github actions by ymarcon · Pull Request #1 · datashield/datashield-opal-python

ymarcon · 2026-02-14T16:25:34Z

No description provided.

Copilot

Pull request overview

This PR migrates the Python project from Poetry to a uv + hatchling toolchain, adds Ruff for linting/formatting, and introduces GitHub Actions workflows for CI, deployment, and stale issue management.

Changes:

Replace Poetry configuration/lockfile with PEP 621 pyproject.toml, uv.lock, and hatchling build backend.
Add Ruff configuration and wire lint/format into Makefile and CI.
Add GitHub Actions workflows for CI, tag-based publish, and stale issues; update tests and implementation formatting/error handling.

Reviewed changes

Copilot reviewed 11 out of 14 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`uv.lock`	Adds uv lockfile to pin dependency resolution.
`pyproject.toml`	Migrates project metadata/deps to PEP 621 and switches build backend to hatchling.
`poetry.lock`	Removes Poetry lockfile as part of the migration.
`Makefile`	Updates local workflows to use uv + ruff and adds build/install targets.
`.ruff.toml`	Introduces Ruff lint/format configuration.
`.gitignore`	Expands ignored local/dev artifacts (e.g., `.venv/`, debug files).
`datashield_opal/impl.py`	Refactors Opal driver implementation (formatting, error chaining, small logic changes).
`datashield_opal/__init__.py`	Minor export adjustment for `OpalDriver`.
`tests/test_impl.py`	Test refactors (style/assertions, improved exception assertions).
`tests/test_api_analysis.py`	Refactors tests and adds error handling around API calls.
`tests/test_api_admin.py`	Refactors tests for style consistency.
`.github/workflows/ci.yml`	Adds CI to install with uv, run Ruff, and run pytest across Python versions.
`.github/workflows/deploy.yml`	Adds tag-triggered build + publish workflow using uv.
`.github/workflows/stale.yml`	Adds automation to mark/close stale issues.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-14T16:29:31Z

.github/workflows/deploy.yml

+      - name: Run tests
+        run: uv run pytest


This workflow runs pytest before publishing, but the tests currently hit https://opal-demo.obiba.org over the network. That makes releases dependent on external service availability and can block publishing due to transient outages. Consider separating unit tests from integration tests and only running integration tests on a dedicated workflow/environment with required secrets.

Suggested change

- name: Run tests

run: uv run pytest

- name: Run unit tests (exclude integration/network tests)

run: uv run pytest -m "not integration"

Copilot · 2026-02-14T16:29:32Z

datashield_opal/impl.py

        def format_method(x):
-            return '%s:%s' % (x['pkg'], x['version'])
+            return f"{x['pkg']}:{x['version']}" if "pkg" in x and "version" in x else None

-        aggregate = [format_method(x) for x in aggregate if 'pkg' in x]
-        assign = [format_method(x) for x in assign if 'pkg' in x]
+        aggregate = [format_method(x) for x in aggregate if "pkg" in x]
+        assign = [format_method(x) for x in assign if "pkg" in x]

        # unique values
        pkgs = list(set(aggregate + assign))

        def format_pkg(x):
-            parts = x.split(':')
-            return {
-                'pkg': parts[0],
-                'version': parts[1]
-            }
+            parts = x.split(":")
+            return {"pkg": parts[0], "version": parts[1]}

        return [format_pkg(x) for x in pkgs]


list_packages() can now produce None entries (when a method dict has pkg but no version) and then format_pkg(x) unconditionally calls x.split(":"), which will crash on None. Filter out None before building pkgs and/or require both pkg and version in the comprehensions.

Copilot · 2026-02-14T16:29:32Z

pyproject.toml

+build-backend = "hatchling.build"
+
+[tool.hatch.build.targets.wheel]
+packages = ["datashield"]


[tool.hatch.build.targets.wheel] packages is set to ["datashield"], but this repo’s actual package directory is datashield_opal/. With the current setting, the built wheel likely won’t include the project code (or will include the wrong module). Update this to include datashield_opal (or rely on hatch’s default package discovery).

Suggested change

packages = ["datashield"]

packages = ["datashield_opal"]

Copilot · 2026-02-14T16:29:32Z

Makefile

+	pip install ./dist/datashield-*.tar.gz
+
+local-install-force: clean build
+	pip install ./dist/datashield-*.tar.gz --break-system-packages


local-install installs ./dist/datashield-*.tar.gz, but the distribution name is datashield-opal (likely producing datashield_opal-*.tar.gz / datashield_opal-*.whl). This target will fail or install the wrong artifact; adjust the glob to match the actual built filename(s).

Suggested change

pip install ./dist/datashield-*.tar.gz

local-install-force: clean build

pip install ./dist/datashield-*.tar.gz --break-system-packages

pip install ./dist/datashield-opal-*.tar.gz

local-install-force: clean build

pip install ./dist/datashield-opal-*.tar.gz --break-system-packages

Copilot · 2026-02-14T16:29:32Z

datashield_opal/impl.py

+        )
        if variables is not None:
-            builder.query('variables','name.any("%s")' % '","'.join(variables))
+            builder.query("variables", f'name.any("{",".join(variables)}")')


The variables query formatting changed from generating name.any("v1","v2") to name.any("v1,v2") (single string). If Opal expects an RQL any() list of values, this change will silently alter filtering and may return incorrect data. Consider restoring the previous quoting/join behavior or generating the exact list syntax Opal expects.

Suggested change

builder.query("variables", f'name.any("{",".join(variables)}")')

builder.query("variables", f'name.any({",".join([f\'"{v}"\' for v in variables])})')

Copilot · 2026-02-14T16:29:32Z

datashield_opal/impl.py

                self.session = response.from_json()
            else:
-                raise OpalDSError(ValueError('DataSHIELD session creation failed: ' + response.code))
+                raise OpalDSError(ValueError("DataSHIELD session creation failed: " + response.code))


"DataSHIELD session creation failed: " + response.code will raise a TypeError because response.code is an int. Convert to string (or use an f-string) so session-creation failures surface as the intended OpalDSError instead of masking the original error.

Suggested change

raise OpalDSError(ValueError("DataSHIELD session creation failed: " + response.code))

raise OpalDSError(ValueError(f"DataSHIELD session creation failed: {response.code}"))

Copilot · 2026-02-14T16:29:33Z

datashield_opal/impl.py

    def keep_alive(self) -> bool:
-        try:
+        with suppress(Exception):
            self.list_symbols()


keep_alive() is annotated to return bool but doesn’t return anything (implicitly None) even on success/failure. Either return True/False (e.g., True if list_symbols() succeeds, else False) or change the return annotation to None to match behavior.

Suggested change

self.list_symbols()

self.list_symbols()

return True

return False

Copilot · 2026-02-14T16:29:33Z

tests/test_api_analysis.py

-
+        try:
+            self.session.assign_resource(
+                "client", resources={"server1": "RSRC.CNSIM1", "server2": "RSRC.CNSIM1"}, asynchronous=False


In test_assign_resources, server2 is assigned RSRC.CNSIM1 (same as server1). This looks inconsistent with test_assign_tables using CNSIM2 for server2 and with the previous version of this test. If the intent is to test multi-server behavior, server2 should likely use RSRC.CNSIM2.

Suggested change

"client", resources={"server1": "RSRC.CNSIM1", "server2": "RSRC.CNSIM1"}, asynchronous=False

"client", resources={"server1": "RSRC.CNSIM1", "server2": "RSRC.CNSIM2"}, asynchronous=False

Copilot · 2026-02-14T16:29:33Z

.github/workflows/ci.yml

+      - name: Run tests
+        run: uv run pytest


CI runs pytest unconditionally, but the current test suite connects to https://opal-demo.obiba.org with real credentials. This makes CI dependent on an external service and likely to be flaky/fail in GitHub Actions. Consider marking these as integration tests (e.g., @pytest.mark.integration) and skipping by default unless an env var/secret is provided, or mock the Opal API for CI.

Suggested change

- name: Run tests

run: uv run pytest

- name: Run unit tests

run: uv run pytest -m "not integration"

- name: Run integration tests (requires Opal credentials)

if: ${{ secrets.OPAL_USERNAME != '' && secrets.OPAL_PASSWORD != '' }}

run: uv run pytest -m "integration"

ymarcon added 2 commits February 14, 2026 13:20

feat: use uv as package manager, ruff as linter, add github actions

ee6a074

fix: tests on resources

7b45a51

ymarcon requested a review from Copilot February 14, 2026 16:25

Copilot started reviewing on behalf of ymarcon February 14, 2026 16:26 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use uv as package manager, ruff as linter, add github actions#1

feat: use uv as package manager, ruff as linter, add github actions#1
ymarcon wants to merge 2 commits intomasterfrom
task/uv

ymarcon commented Feb 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Copilot AI Feb 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

	builder.query("variables", f'name.any("{",".join(variables)}")')
	builder.query("variables", f'name.any({",".join([f\'"{v}"\' for v in variables])})')

	raise OpalDSError(ValueError("DataSHIELD session creation failed: " + response.code))
	raise OpalDSError(ValueError(f"DataSHIELD session creation failed: {response.code}"))

	"client", resources={"server1": "RSRC.CNSIM1", "server2": "RSRC.CNSIM1"}, asynchronous=False
	"client", resources={"server1": "RSRC.CNSIM1", "server2": "RSRC.CNSIM2"}, asynchronous=False

-      - name: Run tests
-        run: uv run pytest
+      - name: Run unit tests
+        run: uv run pytest -m "not integration"
+      - name: Run integration tests (requires Opal credentials)
+        if: ${{ secrets.OPAL_USERNAME != '' && secrets.OPAL_PASSWORD != '' }}
+        run: uv run pytest -m "integration"

Conversation

ymarcon commented Feb 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant