Skip to content

Conversation

@m1so
Copy link
Member

@m1so m1so commented Nov 10, 2025

Summary by CodeRabbit

  • Chores
    • Added a commented version note clarifying compatible matplotlib-inline versions
    • Removed a duplicate dependency declaration for a cleaner configuration
    • Updated CI tooling to a newer patch release and adjusted license-check to ignore matplotlib-inline
    • Added conditional constraints in CI for importlib-metadata when running Python 3.9 tests

@linear
Copy link

linear bot commented Nov 10, 2025

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 10, 2025

📝 Walkthrough

Walkthrough

  • Updated pyproject.toml dependencies: removed a duplicate toml>=0.10.0,<1.0.0 entry and added a commented suggestion for matplotlib-inline>=0.1.7,<0.2.0.
  • CI workflow (.github/workflows/ci.yml) changes: bumped Poetry from 2.2.0 to 2.2.1; added matplotlib-inline to license-check ignore list; added conditional steps across several jobs to constrain importlib-metadata to <8.7 when the Python matrix uses 3.9.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Repo as Repository
participant CI as GitHub Actions CI
participant Job as Matrix Job
participant Step as Job Step

Repo->>CI: push PR triggers workflow
CI->>Job: create matrix (includes Python 3.9)
Note right of Job: Job runs job steps for each matrix entry
Job->>Step: install Poetry (now 2.2.1)
Note right of Step: license-check configured to ignore matplotlib-inline
alt Python == 3.9
Step->>Step: run conditional step to pin importlib-metadata < 8.7
else Python != 3.9
Step->>Step: skip importlib-metadata pin
end
Step->>Job: run remaining checks (spell/format/mypy/tests)

Possibly related PRs

  • deepnote/deepnote-toolkit#203 — similar CI workflow changes affecting Poetry version and Poetry installation steps.

Suggested reviewers

  • hc2p

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title mentions downgrading matplotlib-inline for Python 3.10, but the actual changes include dependency adjustments for Python 3.9 and Poetry version bumps, making the title partially related but not capturing the main scope. Revise title to reflect all key changes: e.g., 'chore(dependencies): Update Poetry and constrain importlib-metadata for Python 3.9'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 59388e5 and 66b5355.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml (3 hunks)
  • pyproject.toml (1 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). (8)
  • GitHub Check: Test - Python 3.13
  • GitHub Check: Test - Python 3.11
  • GitHub Check: Test - Python 3.10
  • GitHub Check: Build and push artifacts for Python 3.9
  • GitHub Check: Build and push artifacts for Python 3.11
  • GitHub Check: Build and push artifacts for Python 3.13
  • GitHub Check: Build and push artifacts for Python 3.10
  • GitHub Check: Build and push artifacts for Python 3.12
🔇 Additional comments (3)
.github/workflows/ci.yml (3)

17-17: Poetry version bump is straightforward.

Update from 2.2.0 → 2.2.1 is clean maintenance.


142-142: matplotlib-inline added to license-check ignore list.

This makes sense if the package is being managed via version markers in pyproject.toml. However, this change depends on resolving the status of line 127 in pyproject.toml (currently commented out).

Confirm that line 127 in pyproject.toml is uncommented and active before merging. If line 127 is removed entirely, this ignore entry may become unnecessary.


174-176: importlib-metadata constraint added to mypy job only; verify coverage for Python 3.9.

The constraint importlib-metadata<8.7 is added conditionally to the mypy job for Python 3.9. However, the tests-unit job (lines 278–340) also has a Python 3.9 matrix entry (line 284) and installs dependencies similarly. Consider whether this constraint should also be applied there for consistency.

Confirm: Does the tests-unit job require the same importlib-metadata<8.7 constraint for Python 3.9, or is the mypy-only placement intentional?


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

@github-actions
Copy link

github-actions bot commented Nov 10, 2025

📦 Python package built successfully!

  • Version: 1.1.2.dev10+1d94949
  • Wheel: deepnote_toolkit-1.1.2.dev10+1d94949-py3-none-any.whl
  • Install:
    pip install "deepnote-toolkit @ https://deepnote-staging-runtime-artifactory.s3.amazonaws.com/deepnote-toolkit-packages/1.1.2.dev10%2B1d94949/deepnote_toolkit-1.1.2.dev10%2B1d94949-py3-none-any.whl"

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between cc6a555 and 9c6b88c.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • pyproject.toml (1 hunks)

@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.39%. Comparing base (cc6a555) to head (66b5355).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #26   +/-   ##
=======================================
  Coverage   75.39%   75.39%           
=======================================
  Files          99       99           
  Lines        5625     5625           
  Branches      784      784           
=======================================
  Hits         4241     4241           
  Misses       1384     1384           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deepnote-bot
Copy link

deepnote-bot commented Nov 10, 2025

🚀 Review App Deployment Started

📝 Description 🌐 Link / Info
🌍 Review application ra-26
🔑 Sign-in URL Click to sign-in
📊 Application logs View logs
🔄 Actions Click to redeploy
🚀 ArgoCD deployment View deployment
Last deployed 2025-11-10 16:41:22 (UTC)
📜 Deployed commit 97d2c19b6c075e58b2e8793edc876bbf544aef9f
🛠️ Toolkit version 1d94949

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0621ece and 1539037.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • pyproject.toml (1 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). (11)
  • GitHub Check: Test - Python 3.13
  • GitHub Check: Test - Python 3.12
  • GitHub Check: Test - Python 3.10
  • GitHub Check: Test - Python 3.11
  • GitHub Check: Typecheck - 3.13
  • GitHub Check: Typecheck - 3.10
  • GitHub Check: Build and push artifacts for Python 3.11
  • GitHub Check: Build and push artifacts for Python 3.12
  • GitHub Check: Build and push artifacts for Python 3.13
  • GitHub Check: Build and push artifacts for Python 3.10
  • GitHub Check: Build and push artifacts for Python 3.9

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 253ddf9 and 59388e5.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml (2 hunks)
  • pyproject.toml (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). (9)
  • GitHub Check: Build and push artifacts for Python 3.9
  • GitHub Check: Build and push artifacts for Python 3.11
  • GitHub Check: Build and push artifacts for Python 3.13
  • GitHub Check: Build and push artifacts for Python 3.12
  • GitHub Check: Build and push artifacts for Python 3.10
  • GitHub Check: Test - Python 3.11
  • GitHub Check: Test - Python 3.13
  • GitHub Check: Test - Python 3.12
  • GitHub Check: Test - Python 3.10
🔇 Additional comments (2)
pyproject.toml (1)

215-215: Poetry version constraint update aligns with CI change.

Line 215 properly downgrades requires-poetry from >=2.2 to >=2.1, matching .github/workflows/ci.yml line 17 (POETRY_VERSION: "2.1.4").

.github/workflows/ci.yml (1)

17-17: Original review comment is incorrect.

matplotlib-inline is a transitive dependency (visible in poetry.lock at multiple locations), not a direct dependency. It's correctly commented out in pyproject.toml because it's not directly required, yet properly appears in the CI ignore list to handle license checks for transitive dependencies pulled in by other packages. No code changes needed.

Likely an incorrect or invalid review comment.

# so we keep vl-convert version pinned as well
"vl-convert-python==1.7.0",
"deepnote-vegafusion>=2.0.8,<3",
# "matplotlib-inline>=0.1.7,<0.2.0", # 0.2.0 is not compatible with matplotlib 3.6.3
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Dependency commented out conflicts with PR intent.

PR title claims "downgrade matplotlib-inline for Python 3.10," but line 127 is entirely commented. Either:

  1. Uncomment with proper constraints (per past reviews):
-    # "matplotlib-inline>=0.1.7,<0.2.0",  # 0.2.0 is not compatible with matplotlib 3.6.3
+    "matplotlib-inline>=0.1.7,<0.2.0; python_version <= '3.10'",  # 0.2.0 incompatible with matplotlib 3.6.3 on Python ≤3.10
+    "matplotlib-inline>=0.2.1,<0.3.0; python_version >= '3.11'",
  1. Or if fully removing: Justify in PR description and remove from CI ignore list.

🏁 Script executed:

#!/bin/bash
# Verify current state of matplotlib-inline in pyproject.toml
echo "=== pyproject.toml around line 127 ==="
sed -n '120,135p' pyproject.toml

echo ""
echo "=== All matplotlib-inline references in repo ==="
rg -n 'matplotlib-inline' --type=toml --type=yaml --type=json

echo ""
echo "=== CI files that might reference dependencies ==="
find . -type f \( -name '*.yml' -o -name '*.yaml' \) -path '*/.github/workflows/*' | head -5

Length of output: 432


🏁 Script executed:

#!/bin/bash
# Correct path and get context
echo "=== pyproject.toml lines 120-140 ==="
sed -n '120,140p' git/pyproject.toml

echo ""
echo "=== Python version markers in pyproject.toml ==="
rg -n 'python_version' git/pyproject.toml | head -10

echo ""
echo "=== matplotlib-inline in CI files ==="
rg -C2 'matplotlib-inline' git/.github/workflows/

echo ""
echo "=== Check if dependencies are pinned/ignored elsewhere ==="
rg -n 'matplotlib|0.2.0|0.1.7' git/pyproject.toml | head -15

Length of output: 428


🏁 Script executed:

#!/bin/bash
# Find and inspect pyproject.toml (with repo cloning)
echo "=== Locate pyproject.toml ==="
fd 'pyproject.toml' -x echo {}

echo ""
echo "=== Lines 120-140 context ==="
fd 'pyproject.toml' -x sed -n '120,140p' {}

echo ""
echo "=== Python version markers in dependencies ==="
fd 'pyproject.toml' -x grep -n 'python_version' {} | head -15

echo ""
echo "=== Nearby matplotlib/ipython deps ==="
fd 'pyproject.toml' -x sed -n '110,160p' {}

Length of output: 4143


Dependency commented out conflicts with PR intent.

PR title claims "downgrade matplotlib-inline for Python 3.10," but line 127 is entirely commented. Either:

  1. Uncomment with proper constraints (per past reviews):
-    # "matplotlib-inline>=0.1.7,<0.2.0",  # 0.2.0 is not compatible with matplotlib 3.6.3
+    "matplotlib-inline>=0.1.7,<0.2.0; python_version <= '3.10'",  # 0.2.0 incompatible with matplotlib 3.6.3 on Python ≤3.10
+    "matplotlib-inline>=0.2.1,<0.3.0; python_version >= '3.11'",
  1. Or if fully removing: Justify in PR description and remove from CI ignore list.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# "matplotlib-inline>=0.1.7,<0.2.0", # 0.2.0 is not compatible with matplotlib 3.6.3
"matplotlib-inline>=0.1.7,<0.2.0; python_version <= '3.10'", # 0.2.0 incompatible with matplotlib 3.6.3 on Python ≤3.10
"matplotlib-inline>=0.2.1,<0.3.0; python_version >= '3.11'",

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.

3 participants