Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hidden windows failures #360

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dalito
Copy link
Contributor

@dalito dalito commented Jan 11, 2025

This fixes two Windows path-related issues that were undetected because of the gh-action.

See runs in #358 for the hidden failures that are fixed by this PR.

Closes linkml/linkml#2490

@@ -304,7 +304,8 @@ def imports_closure(self, imports: bool = True, traverse: Optional[bool] = None,
# we should treat the two `types.yaml` as separate schemas from the POV of the
# origin schema.
if sn.startswith('.') and ':' not in i:
i = os.path.normpath(str(Path(sn).parent / i))
# This cannot be simplified. os.path.normpath() must be called before .as_posix()
i = PurePath(os.path.normpath(PurePath(sn).parent / i)).as_posix()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is probably not the fix we want, if it works - I think we do want to preserve windows paths here rather than casting to as_posix, and the thing that's bugged is my hardcoded string paths in the tests:

assert closure == [
'linkml:types',
'../neighborhood_parent',
'neighbor',
'../parent',
'../L1_0_1/L2_0_1_0/grandchild',
'../../L0_1/L1_1_0/L2_1_0_0/apple',
'../../L0_1/L1_1_0/L2_1_0_0/index',
'../../L0_1/L1_1_0/L2_1_0_1/banana',
'../../L0_1/L1_1_0/L2_1_0_1/index',
'../../L0_1/L1_1_0/index',
'../../L0_1/cousin',
'../L1_0_1/dupe',
'./L2_0_0_0/child',
'./L2_0_0_1/child',
'main'
]

The current test doesn't test for e.g. forward slashes in windows filenames which would get confused with as_posix here, where the reason this seems to work is that it's later rescued into a windows path by hbreader.

I'm not a windows user so idrc but imo we should fix the hardcoded strings in test rather than introduce another layer of path manipulation to schemaview

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking. It was not very clear for me that the test is asserting file paths. In this case we can change the test to use PurePaths instead of strings and revert the change in schemaview.py. Purepath adapts correctly to the platform. Ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I change the test to use PurePaths, The test fails with the original code. The paths are not consistently converted, see test output:

(.venv) λ pytest --lf -vv
================================================= test session starts =================================================
platform win32 -- Python 3.12.8, pytest-8.0.1, pluggy-1.4.0 -- C:\Users\david\MyProg\gh-dalito\linkml-runtime\.venv\Scripts\python.exe
cachedir: .pytest_cache
rootdir: C:\Users\david\MyProg\gh-dalito\linkml-runtime
collected 1 item
run-last-failure: rerun previous 1 failure (skipped 55 files)

tests/test_utils/test_schemaview.py::test_imports_relative FAILED                                                [100%]

====================================================== FAILURES =======================================================
________________________________________________ test_imports_relative ________________________________________________

    def test_imports_relative():
        """Relative imports from relative imports should evaluate relative to the *importing* schema."""
        sv = SchemaView(SCHEMA_RELATIVE_IMPORT_TREE)
        closure = sv.imports_closure(imports=True)

        assert len(closure) == len(sv.schema_map.keys())
>       assert closure == [
            'linkml:types',
            str(PurePath('../neighborhood_parent')),
            'neighbor',
            str(PurePath('../parent')),
            str(PurePath('../L1_0_1/L2_0_1_0/grandchild')),
            str(PurePath('../../L0_1/L1_1_0/L2_1_0_0/apple')),
            str(PurePath('../../L0_1/L1_1_0/L2_1_0_0/index')),
            str(PurePath('../../L0_1/L1_1_0/L2_1_0_1/banana')),
            str(PurePath('../../L0_1/L1_1_0/L2_1_0_1/index')),
            str(PurePath('../../L0_1/L1_1_0/index')),
            str(PurePath('../../L0_1/cousin')),
            str(PurePath('../L1_0_1/dupe')),
            str(PurePath('./L2_0_0_0/child')),
            str(PurePath('./L2_0_0_1/child')),
            'main'
        ]
E       AssertionError: assert ['linkml:types', '../neighborhood_parent', 'neighbor', '..\\L1_0_1\\L2_0_1_0\\grandchild', '..\\parent', '../parent', '..\\..\\L0_1\\L1_1_0\\L2_1_0_0\\apple', '..\\..\\L0_1\\L1_1_0\\L2_1_0_0\\index', '..\\..\\L0_1\\L1_1_0\\L2_1_0_1\\banana', '..\\..\\L0_1\\L1_1_0\\L2_1_0_1\\index', '..\\..\\L0_1\\L1_1_0\\index', '../../L0_1/cousin', '..\\L1_0_1\\dupe', './L2_0_0_0/child', './L2_0_0_1/child', 'main'] == ['linkml:types', '..\\neighborhood_parent', 'neighbor', '..\\parent', '..\\L1_0_1\\L2_0_1_0\\grandchild', '..\\..\\L0_1\\L1_1_0\\L2_1_0_0\\apple', '..\\..\\L0_1\\L1_1_0\\L2_1_0_0\\index', '..\\..\\L0_1\\L1_1_0\\L2_1_0_1\\banana', '..\\..\\L0_1\\L1_1_0\\L2_1_0_1\\index', '..\\..\\L0_1\\L1_1_0\\index', '..\\..\\L0_1\\cousin', '..\\L1_0_1\\dupe', 'L2_0_0_0\\child', 'L2_0_0_1\\child', 'main']
E
E         At index 1 diff: '../neighborhood_parent' != '..\\neighborhood_parent'
E         Left contains one more item: 'main'
E
E         Full diff:
E           [
E               'linkml:types',
E         -     '..\\neighborhood_parent',
E         ?        ^^
E         +     '../neighborhood_parent',
E         ?        ^
E               'neighbor',
E         +     '..\\L1_0_1\\L2_0_1_0\\grandchild',
E               '..\\parent',
E         -     '..\\L1_0_1\\L2_0_1_0\\grandchild',
E         +     '../parent',
E               '..\\..\\L0_1\\L1_1_0\\L2_1_0_0\\apple',
E               '..\\..\\L0_1\\L1_1_0\\L2_1_0_0\\index',
E               '..\\..\\L0_1\\L1_1_0\\L2_1_0_1\\banana',
E               '..\\..\\L0_1\\L1_1_0\\L2_1_0_1\\index',
E               '..\\..\\L0_1\\L1_1_0\\index',
E         -     '..\\..\\L0_1\\cousin',
E         ?        ^^  ^^    ^^
E         +     '../../L0_1/cousin',
E         ?        ^  ^    ^
E               '..\\L1_0_1\\dupe',
E         -     'L2_0_0_0\\child',
E         ?              ^^
E         +     './L2_0_0_0/child',
E         ?      ++        ^
E         -     'L2_0_0_1\\child',
E         ?              ^^
E         +     './L2_0_0_1/child',
E         ?      ++        ^
E               'main',
E           ]

tests\test_utils\test_schemaview.py:527: AssertionError
-------------------------------------------------- Captured log call --------------------------------------------------
WARNING  rdflib.term:term.py:278 C:\Users\david\MyProg\gh-dalito\linkml-runtime\linkml_runtime\linkml_model\model\schema\types does not look like a valid URI, trying to serialize this will break.
================================================== warnings summary ===================================================
.venv\Lib\site-packages\pydantic\_internal\_config.py:272
  C:\Users\david\MyProg\gh-dalito\linkml-runtime\.venv\Lib\site-packages\pydantic\_internal\_config.py:272: PydanticDeprecatedSince20: Support for class-based `config` is deprecated, use ConfigDict instead. Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.6/migration/
    warnings.warn(DEPRECATION_MESSAGE, DeprecationWarning)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=============================================== short test summary info ===============================================
FAILED tests/test_utils/test_schemaview.py::test_imports_relative - AssertionError: assert ['linkml:types', '../neighborhood_parent', 'neighbor', '..\\L1_0_1\\L2_0_1_0\\grandchild', '...
============================================ 1 failed, 1 warning in 1.23s =============================================

Copy link
Contributor Author

@dalito dalito Jan 14, 2025

Choose a reason for hiding this comment

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

@sneakers-the-rat The method SchemaView.imports_closure relies on consistent path-symbol "/". So I think we should stick with the proposed fix. But I will limit it to windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok that inconsistency does make me concerned about my implementation there though, something here is wrong and i think it's my fault, so i won't hold this up but i do want to remember to follow up on it because it shouldn't be the case that we sometimes get forward slashes and sometimes don't out of schemaview.

the only thing i would be concerned with is the as_posix part, where if idk there is some windows path that can't be roundtripped to posix for some reason, but i don't know enough about windows paths and in fact don't use windows at all so i am the wrong person to be thinking about that.

Copy link
Contributor Author

@dalito dalito Jan 15, 2025

Choose a reason for hiding this comment

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

The conversion with as_posix is limited to some paths with if sn.startswith('.') and ':' not in i: and so it should not cause issues. I agree with respect to a hidden inconsistency in the implementation.

From my perspective this should be a good enough to merge.

@pkalita-lbl, you are also in the windows-wg. Do you have time for a review?

@dalito dalito force-pushed the issue2490-fix-hidden-windows-failures branch from c7749c6 to b264c8e Compare January 14, 2025 21:55
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.

[linkml-runtime] Hidden Windows failures in gh-actions
2 participants