Ensure that relative imports can be imported without requiring `./` in front of the import file name #350

ialarmedalien · 2024-11-22T17:24:14Z

Ensures that relative imports work correctly without having ./ in front of the import file name.

ialarmedalien · 2025-01-31T20:23:14Z

@sierra-moxon any chance of a review? should be pretty simple.

sneakers-the-rat · 2025-02-10T03:29:37Z

linkml_runtime/utils/schemaview.py

@@ -303,7 +303,7 @@ def imports_closure(self, imports: bool = True, traverse: Optional[bool] = None,
                    # - subdir/types.yaml
                    # we should treat the two `types.yaml` as separate schemas from the POV of the
                    # origin schema.
-                    if sn.startswith('.') and ':' not in i:
+                    if '/' in sn and ':' not in i:


I think you want an additional check for something being an absolute path here:

Suggested change

if '/' in sn and ':' not in i:

if '/' in sn and ':' not in i and not PurePath(sn).is_absolute():

but otherwise this seems fine. the ':' check still protects against this breaking other protocol schemes.

only downside i can think of are that by allowing relative paths without leading ./ or ../, they become less explicit, but shouldn't affect the way imports are normally used now. maybe a tiny chance of some weird json <-> yaml escaped slash problem, but that would be a json parsing bug.

Can you suggest a test case to add where the importer will fail without the test for an absolute path? I tried to add as many variants of path representation as possible to the tests so I'd like to have at least one path that would fail if the suggested addition was not in place.

sure - should be any second-level schema imported via absolute path. one second

oh! pleasantly surprised to learn that an absolute path causes the thing it's being appended to to be ignored. learned something new :). Disregard this.

Thanks for checking - glad I could provide a surprising learning experience for the day!

dalito · 2025-02-10T12:14:33Z

It seems that it breaks Windows but you don't see it because #360 is not yet merged.

Also running your PR-branch locally on Win10 gives an error:

(.venv) λ pytest -k schemaview
======================================= test session starts =======================================
platform win32 -- Python 3.10.11, pytest-8.0.1, pluggy-1.4.0
rootdir: C:\Users\dlinke\MyProg_local\gh-dalito\linkml-runtime
collected 663 items / 630 deselected / 33 selected

tests\test_issues\test_linkml_runtime_issue_1317.py .s.                                      [  9%]
tests\test_utils\test_schemaview.py ............F.s...............                           [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)

tests\test_utils\test_schemaview.py:524:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
linkml_runtime\utils\schemaview.py:281: in imports_closure
    imported_schema = self.load_import(sn)
linkml_runtime\utils\schemaview.py:227: in load_import
    schema = load_schema_wrap(sname + '.yaml', base_dir=base_dir)
linkml_runtime\utils\schemaview.py:84: in load_schema_wrap
    schema = yaml_loader.load(path, target_class=SchemaDefinition, **kwargs)
linkml_runtime\loaders\loader_root.py:76: in load
    results = self.load_any(*args, **kwargs)
linkml_runtime\loaders\yaml_loader.py:41: in load_any
    data_as_dict = self.load_as_dict(source, base_dir=base_dir, metadata=metadata)
linkml_runtime\loaders\yaml_loader.py:27: in load_as_dict
    data = self._read_source(source, base_dir=base_dir, metadata=metadata, accept_header="text/yaml, application/yaml;q=0.9")
linkml_runtime\loaders\loader_root.py:167: in _read_source
    data = hbread(source, metadata, base_dir, accept_header)
.venv\lib\site-packages\hbreader\__init__.py:260: in hbread
    with hbopen(source, open_info, base_path, accept_header, is_actual_data, read_codec) as f:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

source = './four.yaml'
open_info = FileInfo(source_file=None, source_file_date=None, source_file_size=None, base_path='C:\\Users\\dlinke\\MyProg_local\\gh-dalito\\linkml-runtime\\tests\\test_utils\\input\\imports_relative\\L0_0\\L1_0_0')
base_path = 'C:\\Users\\dlinke\\MyProg_local\\gh-dalito\\linkml-runtime\\tests\\test_utils\\input\\imports_relative\\L0_0\\L1_0_0'
accept_header = 'text/yaml, application/yaml;q=0.9'
is_actual_data = <function default_str_tester at 0x00000199BCE57760>, read_codec = None

    def hbopen(source: HB_TYPE,
               open_info: Optional[FileInfo] = None,
               base_path: Optional[str] = None,
               accept_header: Optional[str] = None,
               is_actual_data: Optional[Callable[[str], bool]] = default_str_tester,
               read_codec: str = None) -> TextIO:
        """
        Return an open IO representation of source
        :param source: anything that can be construed to be a string, a URL, a file name or an open file handle
        :param open_info: what we learned about source in the process of converting it
        :param base_path: Base to use if source is a relative URL or file name
        :param accept_header: Accept header to use if it turns out to be a URL
        :param is_actual_data: Function to differentiate plain text from URL or file name
        :param read_codec: Name of codec to use if bytes being read. (URL only)
        :return: TextIO representation of open file
        """
        source_type = detect_type(source, base_path, is_actual_data)
        if source_type is HBType.STRINGABLE:
            source_as_string = str(source)
        elif source_type is HBType.DECODABLE:
            # TODO: Tie this into the autodetect machinery
            source_as_string = source.decode()
        elif source_type is HBType.STRING:
            source_as_string = source
        else:
            source_as_string = None

        # source is a URL or a file name
        if source_as_string:
            if open_info:
                assert open_info.source_file is None, "source_file parameter not allowed if data is a file or URL"
                assert open_info.source_file_date is None, "source_file_date parameter not allowed if data is a file or URL"
                open_info.source_file_size = len(source_as_string)
            return StringIO(source_as_string)

        if source_type is HBType.URL:
            url = source if '://' in source else urljoin(base_path + ('' if base_path.endswith('/') else '/'),
                                                         source, allow_fragments=True)
            req = Request(quote(url, '/:'))
            if accept_header:
                req.add_header("Accept", accept_header)
            try:
                response = urlopen(req, context=ssl._create_unverified_context())
            except HTTPError as e:
                # This is here because the message out of urllib doesn't include the file name
                e.msg = f"{e.filename}"
                raise e
            if open_info:
                open_info.source_file = response.url
                open_info.source_file_date = response.headers['Last-Modified']
                if not open_info.source_file_date:
                    open_info.source_file_date = response.headers['Date']
                open_info.source_file_size = response.headers['Content-Length']
                parts = urlsplit(response.url)
                open_info.base_path = urlunsplit((parts.scheme, parts.netloc, os.path.dirname(parts.path),
                                                 parts.query, None))
            # Auto convert byte stream to
            return _to_textio(response, response.fp.mode, read_codec)

        if source_type is HBType.FILENAME:
            if not base_path:
                fname = os.path.abspath(source)
            else:
                fname = source if os.path.isabs(source) else os.path.abspath(os.path.join(base_path, source))
>           f = open(fname, encoding=read_codec if read_codec else 'utf-8')
E           FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\dlinke\\MyProg_local\\gh-dalito\\linkml-runtime\\tests\\test_utils\\input\\imports_relative\\L0_0\\L1_0_0\\four.yaml'

.venv\lib\site-packages\hbreader\__init__.py:206: FileNotFoundError

===================================== short test summary info =====================================
FAILED tests/test_utils/test_schemaview.py::test_imports_relative - FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\dlinke\\MyProg_local\\gh-da...
============== 1 failed, 30 passed, 2 skipped, 630 deselected, 37 warnings in 9.81s ===============

ialarmedalien · 2025-02-10T22:16:56Z

@dalito is #360 going to be merged? Not sure what the order of precedence of these PRs is.

dalito · 2025-02-10T23:29:57Z

I have asked on dev-channel if #360 can be merged.

cmungall · 2025-02-13T03:08:11Z

#360 now merged! I think it's best if one of you two handle the resulting conflict...

ialarmedalien · 2025-02-18T16:52:44Z

@dalito it looks like the GA tests passed; would you be able to see if they pass locally on Win10? I don't have a windows box available.

dalito · 2025-02-18T17:43:59Z

@ialarmedalien - The tests pass now on Win10. Great!

sneakers-the-rat

thanks for fixing this. the ./ check always felt fragile to me. always love it when the fix is so simple because the stdlib tools are better than i knew they were!

no idea why upstream tests are failing, but doesn't look related to this. approving for the substance of the PR and hopefully that's uh just a temporary glitch

ialarmedalien · 2025-02-21T16:25:42Z

I don't have merge privs so can someone who does press the magic button (or make a mental note to press the magic button if there are other PRs that depend on this)?

@dalito @sneakers-the-rat

sierra-moxon · 2025-02-21T17:23:22Z

Before we merge, lets make sure we address the upstream failing tests (as an out here, we could post a note to why they are failing in this PR and we can get someone to fix separately). (been burned so many times by merging without the green checks that I am def hesitant to do that - esp on Fridays! :P )

sneakers-the-rat · 2025-02-21T23:14:56Z

the failures are truly mysterious to me, it's swapping out owl:DataRange for rdfs:Datatype ... and i can't fathom how this PR would have affected that (except for on windows in python 3.12). can anyone with privs trigger a re-run of the failed actions to see if it was a weird fluke?

dalito · 2025-02-21T23:35:38Z

I just triggered a re-run of failed jobs. Let's see.

dalito · 2025-02-22T00:10:51Z

The mysterious errors are gone. The remaining errors are probably related to the changes made in #357

ialarmedalien force-pushed the bugfix/issue-2422 branch 2 times, most recently from 151aa7c to adb8ed1 Compare January 31, 2025 20:36

sierra-moxon self-requested a review February 4, 2025 17:54

sierra-moxon approved these changes Feb 4, 2025

View reviewed changes

sneakers-the-rat reviewed Feb 10, 2025

View reviewed changes

This was referenced Feb 10, 2025

Fix multilayer relative imports #368

Open

Fix test upstream: -y to pip uninstall #369

Merged

ialarmedalien force-pushed the bugfix/issue-2422 branch from adb8ed1 to 30b2186 Compare February 13, 2025 21:25

allow relative imports to be imported without ./ at the start

f5d5961

ialarmedalien force-pushed the bugfix/issue-2422 branch from 30b2186 to f5d5961 Compare February 13, 2025 21:44

sneakers-the-rat approved these changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that relative imports can be imported without requiring `./` in front of the import file name #350

Ensure that relative imports can be imported without requiring `./` in front of the import file name #350

ialarmedalien commented Nov 22, 2024

ialarmedalien commented Jan 31, 2025 •

edited

Loading

sneakers-the-rat Feb 10, 2025

ialarmedalien Feb 18, 2025

sneakers-the-rat Feb 19, 2025

sneakers-the-rat Feb 20, 2025

ialarmedalien Feb 20, 2025

dalito commented Feb 10, 2025 •

edited

Loading

ialarmedalien commented Feb 10, 2025

dalito commented Feb 10, 2025

cmungall commented Feb 13, 2025

ialarmedalien commented Feb 18, 2025

dalito commented Feb 18, 2025

sneakers-the-rat left a comment

ialarmedalien commented Feb 21, 2025

sierra-moxon commented Feb 21, 2025

sneakers-the-rat commented Feb 21, 2025

dalito commented Feb 21, 2025

dalito commented Feb 22, 2025

	if '/' in sn and ':' not in i:
	if '/' in sn and ':' not in i and not PurePath(sn).is_absolute():

Ensure that relative imports can be imported without requiring ./ in front of the import file name #350

Are you sure you want to change the base?

Ensure that relative imports can be imported without requiring ./ in front of the import file name #350

Conversation

ialarmedalien commented Nov 22, 2024

ialarmedalien commented Jan 31, 2025 • edited Loading

sneakers-the-rat Feb 10, 2025

Choose a reason for hiding this comment

ialarmedalien Feb 18, 2025

Choose a reason for hiding this comment

sneakers-the-rat Feb 19, 2025

Choose a reason for hiding this comment

sneakers-the-rat Feb 20, 2025

Choose a reason for hiding this comment

ialarmedalien Feb 20, 2025

Choose a reason for hiding this comment

dalito commented Feb 10, 2025 • edited Loading

ialarmedalien commented Feb 10, 2025

dalito commented Feb 10, 2025

cmungall commented Feb 13, 2025

ialarmedalien commented Feb 18, 2025

dalito commented Feb 18, 2025

sneakers-the-rat left a comment

Choose a reason for hiding this comment

ialarmedalien commented Feb 21, 2025

sierra-moxon commented Feb 21, 2025

sneakers-the-rat commented Feb 21, 2025

dalito commented Feb 21, 2025

dalito commented Feb 22, 2025

Ensure that relative imports can be imported without requiring `./` in front of the import file name #350

Ensure that relative imports can be imported without requiring `./` in front of the import file name #350

ialarmedalien commented Jan 31, 2025 •

edited

Loading

dalito commented Feb 10, 2025 •

edited

Loading