Skip to content

Conversation

@amc-corey-cox
Copy link
Contributor

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.

@amc-corey-cox
Copy link
Contributor Author

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.

@amc-corey-cox amc-corey-cox requested a review from Copilot May 14, 2025 20:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 JsonObj and refines test assertions in the RDFS importer tests.
  • Extends CSV generalizer to distinguish date vs. datetime types with new tests.
  • Improves RdfsImportEngine with 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 pytest import 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" ]
Copy link

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] There’s an extra space before the comma between "3.12" and "3.13"; removing it will improve YAML readability.

Suggested change
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.
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.")
Copy link

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] You’re using warnings.warn here but logging.warning elsewhere; consider using a consistent logging approach for uniformity.

Suggested change
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.
# -*- coding: utf-8 -*-

"""Test the module can be imported."""
# Monkey patching jsonobj to fix windows issue
Copy link
Member

@dalito dalito May 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@amc-corey-cox amc-corey-cox merged commit e31924f into main May 19, 2025
11 checks passed
@amc-corey-cox amc-corey-cox deleted the fix_win_164 branch May 19, 2025 17:57
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.

4 participants