Add back windows workflow testing #168
Conversation
|
Okay, well this is an ugly monkey patch to get this to go through. But it is successful and I think this saves us from having to just disable windows latest. I don't really know what is going on here other than it is not a Schema Automator issue. I'll file an issue with linkml-runtime and see if they have a fix. |
There was a problem hiding this comment.
Pull Request Overview
Introduces Windows compatibility in tests, enhances type inference for dates, and refines the RDFS import engine, while expanding CI coverage and updating project configuration.
- Adds a Windows-specific monkey patch for
JsonObjand refines test assertions in the RDFS importer tests. - Extends CSV generalizer to distinguish
datevs.datetimetypes with new tests. - Improves
RdfsImportEnginewith stronger type hints, duplicate‐slot warnings, and normalization of slot ranges. - Updates GitHub Actions to include more Python versions on Windows and migrates dev-dependencies to Poetry’s new group syntax.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_importers/test_rdfs_importer.py | Added Windows monkey patch for JsonObj, updated and removed some assertions |
| tests/test_generalizers/test_csv_data_generalizer.py | Added date and datetime cases to infer_range tests |
| schema_automator/importers/rdfs_import_engine.py | Enhanced signatures, added warnings for duplicate slots, and slot‐range normalization |
| schema_automator/generalizers/csv_data_generalizer.py | Inserted date vs. datetime logic before measurement detection |
| pyproject.toml | Migrated [tool.poetry.dev-dependencies] to [tool.poetry.group.dev.dependencies] |
| .github/workflows/check-pull-request.yaml | Expanded OS/Python matrix, allowed Python 3.13 failures, updated cache action |
Comments suppressed due to low confidence (2)
tests/test_importers/test_rdfs_importer.py:34
- Several assertions for class/slot counts were removed, reducing the test’s coverage of the importer’s output structure; consider re-adding targeted assertions to maintain coverage.
def test_import_foaf():
tests/test_importers/test_rdfs_importer.py:19
- [nitpick] The
pytestimport is unused in this unittest-based file; you can remove it to avoid confusion.
import pytest
| exclude: | ||
| - os: windows-latest | ||
| python-version: "3.9" | ||
| python-version: [ "3.9", "3.10", "3.11", "3.12" , "3.13" ] |
There was a problem hiding this comment.
[nitpick] There’s an extra space before the comma between "3.12" and "3.13"; removing it will improve YAML readability.
| python-version: [ "3.9", "3.10", "3.11", "3.12" , "3.13" ] | |
| python-version: [ "3.9", "3.10", "3.11", "3.12", "3.13" ] |
Copilot uses AI. Check for mistakes.
| @@ -130,7 +131,10 @@ def convert( | |||
| cls_slots = defaultdict(list) | |||
|
|
|||
| for slot in self.generate_rdfs_properties(g, cls_slots): | |||
| sb.add_slot(slot) | |||
| if slot.name in sb.schema.slots: | |||
| warnings.warn(f"Slot '{slot.name}' already exists in schema; skipping duplicate.") | |||
There was a problem hiding this comment.
[nitpick] You’re using warnings.warn here but logging.warning elsewhere; consider using a consistent logging approach for uniformity.
| warnings.warn(f"Slot '{slot.name}' already exists in schema; skipping duplicate.") | |
| logging.warning(f"Slot '{slot.name}' already exists in schema; skipping duplicate.") |
Copilot uses AI. Check for mistakes.
| @@ -1,6 +1,17 @@ | |||
| # -*- coding: utf-8 -*- | |||
|
|
|||
| """Test the module can be imported.""" | |||
| # Monkey patching jsonobj to fix windows issue | |||
There was a problem hiding this comment.
Instead of the monkey patch, I suggest to just skip the test on windows by marking it with
@pytest.mark.skipif(sys.platform == "win32", reason="Does not run on windows untill linkml-runtime is fixed (https://github.com/linkml/linkml-runtime/pull/391)")
With the proposed fix for linkml-runtime the tests pass without the monkey-patch on Windows (I tested locally).
So this "fix" is only needed for a short while.
There was a problem hiding this comment.
I worry about missing something else in the test during the window that we aren't testing due to waiting on linkml-runtime. I'm probably be incredibly defensive in worrying about that. Either way, we have to remove these as soon as we migrate to the new linkml-runtime.
There was a problem hiding this comment.
To be clear... you're probably right that this is the better simpler solution but since I've already done the monkey-patch I'm inclined to leave it in defensively.
We want to make sure we're also compatible with windows in our workflow. This adds back testing and we'll hopefully figure out how to solve the issues.