Make the model name generation more robust in JSON schema

[joakimnordling]

Change Summary

This fixes an issue where the JSON schema can end up using the drive letter (like C, D etc) instead of the model name on Windows if the module with the model is imported from an absolute path on the filesystem (for example using importlib). This was caused by splitting at the first colon : character to remove it and the ID following it, thus on an absolute path on Windows of the form C:\... ended up becoming just the drive letter. This fix changes it to remove the LAST colon character and anything following it.

Modified description post-further investigation of the issue / source of the problem:

This makes the model name used in the JSON Schema be correct also if the model is loaded from a module that has a colon in the name. This should not really happen in any correct use case (python modules should not typically have colons in them), but it's possible to achieve by accident if you use the absolute file path as the module name on Windows when working with importlib. In that case you would have ended up getting just the drive letter as a model name in the JSON Schema, this fix ensures it will still work correctly in that case.

Related issue number

fix #7860

Checklist

• The pull request title is a good summary of the changes - it will be used in the changelog
• Unit tests for the changes exist
• Tests pass on CI
• Documentation reflects the changes where applicable
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

[sydney-runkle]

Please update 👍
As discussed in the issue, with a unit test 😄

[joakimnordling]

I think I should have put this here, rather than in the issue (well, it's now in both):

Hi,

Sorry for the delay with the unittest and the PR in general. I ended up checking a bit deeper into the issue still while adding the unittest and realized a mistake in the way importlib was used and then got interrupted with some more urgent work for couple of days.

This was the original proof of concept code (based on our project that was using it the same way):

    poc_dir = Path(__file__).parent.relative_to(Path.cwd())
    p = (poc_dir / "models.py").absolute()
    spec = importlib.util.spec_from_file_location(name=str(p), location=str(p))

    if not spec.loader:
        raise RuntimeError(f"Failed to import {p} module")

    module = spec.loader.load_module(str(p))

    return getattr(module, "MyModel")

An import of a module file should really be made like this instead: https://docs.python.org/3/library/importlib.html#importing-a-source-file-directly

But if one uses the way shown above it will cause the name of the module to also be something like C:\path\to\module.py (on Windows). I was originally trying to change the name parameter to something without a colon, but each try to do that resulted in an error. That way of doing the import will only work if the name and location match (based on empirical tests at least), whereas when using the correct way you can set the name to something that looks like a python module name.

Somehow I didn't figure out that this was the wrong way to do the import and what the right way was.

I think the fix itself still makes sense; it makes the code more robust, but on the other hand I'm not sure if I really think the unit test for it makes sense; one should not really name a python module with a colon in the name. Showing that being done in the unittest and cluttering one of the fixtures with an extra parameter is not something I really feel happy about. On the other hand I can't think of any better way to add the test without unnecessary duplication either.

[joakimnordling]

I think this PR should be renamed to Make the model name generation more robust in JSON schema and the description something like this:

---

This makes the model name used in the JSON Schema be correct also if the model is loaded from a module that has a colon in the name. This should not really happen in any correct use case (python modules should not typically have colons in them), but it's possible to achieve by accident if you use the absolute file path as the module name on Windows when working with importlib. In that case you would have ended up getting just the drive letter as a model name in the JSON Schema, this fix ensures it will still work correctly in that case.

[sydney-runkle]

Looks good to me. As you said, making the selection of the components more robust is a good change to make. I agree that this is a pretty rare edge case for the test, but given that it captures the value of the change to json_schema.py, I'm ok with it!

@joakimnordling, thanks for the contribution!