Use on-the-fly rewrite (no more file writes)

[dhanak]

I really like this package, but IMHO the rewriting of the extension packages could be done without writing to the file system, providing a more robust approach. Here's my take on it.

[pat-alt]

I also really like this package and I like this PR.

I think that avoiding the file write could help with the fact that currently, you cannot split and organise the files in ext into various subfiles. I originally did this in this PR:

ext
├── MPIExt
│   ├── MPIExt.jl
│   └── utils.jl
├── PythonCallExt
│   ├── PythonCallExt.jl
│   ├── models.jl
│   └── utils.jl
└── RCallExt
    ├── RCallExt.jl
    └── utils.jl

Using this setup tests failed on Julia versions <1.9 after warnings such as the one below that certain extension files couldn't be located:

┌ Warning: Error requiring `MPI` from `CounterfactualExplanations`
│   exception =
│    LoadError: SystemError: opening file "/home/runner/work/CounterfactualExplanations.jl/CounterfactualExplanations.jl/ext_compat/MPIExt/utils.jl": No such file or directory
│    Stacktrace:
[...]

Eventually, I moved all code into single files

ext
├── MPIExt.jl
├── PythonCallExt.jl
└── RCallExt.jl

and all tests passed, but the previous folder structure was more organised.

[cjdoris]

@dhanak Thanks for the PR. This is actually how an early version (pre v1) of the package worked, but the 2-argument version of include was introduced in Julia 1.5 - hence the tests are failing on earlier versions. Since this is a compat package, I wanted it to work all the way back to Julia 1.0.

So there are a few options:

1. The current approach: rewrite the code to disk.
2. Only support Julia 1.5+ and rewrite on the fly.
3. Use both strategies depending on the Julia version.

I'd like to avoid 3 since it's more code to support and test - and might introduce subtle differences in behaviour between versions.

Arguably option 2 is ok since Julia 1.6 is LTS. It would be informative to look at the Julia bound in packages using PackageExtensionCompat.

[cjdoris]

@pat-alt Your issue is that only the root file for each extension gets rewritten currently. It would be straightforward to copy all the ext code over - could you make a separate issue for that please?

[dhanak]

Yes, now I can see from the tests that the two parameter include doesn't work before Julia 1.5. My mistake, but still a pity.

Perhaps this on-the-fly variant of the PackageExtensionCompat can be versioned 2.0, and it would be compatible with Julia versions 1.5 (or 1.6 LTS) up only. The pre-1.5 Julia support could still be supported by the 1.0 branch. I doubt that a lot of maintenance will be required for this code base. This way, the package manager would automatically pick up the best version for any Julia release.

WDYT?

[cjdoris]

Yeah I considered that option, but I don't think it's really any different from option 3 above (branch on the Julia version) - the end result is the same and the amount of code to support is about the same. Personally I'd prefer option 3 - since then at least all the code is in one place.

A disadvantage of your option 4 is that somebody may reasonably put a compat bound PackageExtensionCompat = "2" in their package and now it won't work on Julia 1.0.

Is there any real objection to the status quo other than that icky feeling you get from rewriting code to disk just to immediately read it in again?

[dhanak]

Yeah I considered that option, but I don't think it's really any different from option 3 above (branch on the Julia version) - the end result is the same and the amount of code to support is about the same. Personally I'd prefer option 3 - since then at least all the code is in one place.

A slight benefit would be that the code itself would be clearer in either variant, but you have a fair point.

A disadvantage of your option 4 is that somebody may reasonably put a compat bound PackageExtensionCompat = "2" in their package and now it won't work on Julia 1.0.

This is all hypothesizing, but IMHO, if one were to maintain backward compatibility with Julia 1.0, one would surely check the package versions installed by Julia 1.0, where it would be immediately clear that PackageExtensionCompat v1 is being used.

Is there any real objection to the status quo other than that icky feeling you get from rewriting code to disk just to immediately read it in again?

For one, you need to add ext_compat to .gitignore. And at least on one occasion, when I tested my package with various Julia versions and deleted the ext_compat directory several times in the process, at one point, it just complained about it missing, but didn't regenerate it. I don't know what the exact steps were, and I think it got resolved by deleting the Manifest.toml file as well and then re-resolving the dependencies. It was just a minor nuisance.

So overall, I don't really mind if everything stays as is, and it's your call to make, anyhow.

[dhanak]

That being said, I came up with a workaround that makes the parsing solution viable also for pre 1.5 versions. See the updated PR, please.

[cjdoris]

IIUC this PR now implements the 3-arg version of include itself? Does it work if the extension has more than one source file (i.e. if the main extension file includes another one)? The tests don't cover this case but I suspect the inner includes will break, because the relative file names won't be resolvable. (Admittedly the current version also has this limitation but it's easy to fix.)

[cjdoris]

This is all hypothesizing, but IMHO, if one were to maintain backward compatibility with Julia 1.0, one would surely check the package versions installed by Julia 1.0, where it would be immediately clear that PackageExtensionCompat v1 is being used.

I think you're right, this particular objection would be caught by proper testing.

For one, you need to add ext_compat to .gitignore. And at least on one occasion, when I tested my package with various Julia versions and deleted the ext_compat directory several times in the process, at one point, it just complained about it missing, but didn't regenerate it. I don't know what the exact steps were, and I think it got resolved by deleting the Manifest.toml file as well and then re-resolving the dependencies. It was just a minor nuisance.

How about this package also writes out a ext_compat/.gitignore to ignore everything? Should solve your first issue. If your second issue (missing extension code) is reproducible please file an issue.

So overall, I don't really mind if everything stays as is, and it's your call to make, anyhow.

I'm leaning this way - the current approach is nice and simple. I'd certainly prefer doing the transformation in-memory but without the 2-arg include I think it'll be hard to get right in general (we'd be reimplementing include ourselves).

[dhanak]

IIUC this PR now implements the 3-arg version of include itself? Does it work if the extension has more than one source file (i.e. if the main extension file includes another one)? The tests don't cover this case but I suspect the inner includes will break, because the relative file names won't be resolvable. (Admittedly the current version also has this limitation but it's easy to fix.)

You are perfectly right. It doesn't take care of inner includes, just as the current version, as you point it out. But this can be taken care of. While fixing this, I also noticed that neither implementation works well if the extension has a submodule and makes use of the dependent package from there. The use ..WeakDep form doesn't work in this case, because the .. is insufficient.

I fixed all these in the PR, please see the updates.

[cjdoris]

Thanks for adding tests for submodules and includes. I'm now quite in favour of this PR. There's a few comments to address.

[codecov]

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

[cjdoris]

Thanks very much for this!