ACEsuit / ACEfit.jl

Generic Codes for Fitting ACE models
MIT License
7 stars 6 forks source link

Python dependencies #38

Closed cortner closed 1 year ago

cortner commented 1 year ago

Python dependencies are ok but should not be required. Is this currently guaranteed?

cortner commented 1 year ago

@tjjarvinen and @wcwitt -- I just ran into this again. Have you used @require before? James Gardner once showed me how to use it to load some Julia code only if some package has been imported elsewhere already. Or at least I think that's how it works.

I wonder whether to bring this to ACEfit so that anything PyCall related will only be loaded if PyCall has already been loaded outside? What do you think?

wcwitt commented 1 year ago

I'm trying to understand the precise issue. Is it that ACEfit has PyCall as a dependency in its Project.toml? And this is a problem because someone without Python would have trouble installing PyCall?

Looking at Requires.jl, I could imagine us putting a @require in front of all the functions that use sklearn. But what does that achieve exactly ... wouldn't ACEfit still need to depend on PyCall?

https://github.com/JuliaPackaging/Requires.jl

I might be missing the point still.

tjjarvinen commented 1 year ago

I have used Requires.jl.

In short you can use it, by adding __init__() to main file of the package. Here is an example that I used.

function __init__()
    @require CUDA="052768ef-5323-5732-b1bb-66c8b64840ba" include("cuda_additions.jl")
end

TensorOperations.jl has a more complicated example.

You need to explicitly load the package before for it to trigger @require. In this case it would be (e.g.)

using PyCall
using ACEfit  # now triggers @require
wcwitt commented 1 year ago

I think I understand that - and if it will solve @cortner's problem, great - but what confuses me is that TensorOperations.jl still has CUDA in its Project.toml dependencies.

So if we did that, wouldn't

]add ACEfit

still trigger installation of PyCall?

wcwitt commented 1 year ago

Okay, from the Requires.jl readme:

@required packages can be added to the test environment of a Julia project for integration tests, or directly to the project to document compatible versions in the [compat] section of Project.toml.

which seems to imply that we wouldn't actually need PyCall in the ACEfit dependencies (although we could have it if we wanted to).

So this does seem like it would solve the issue.

wcwitt commented 1 year ago

I hope this will be addressed by 558adebc747f2c9f5300d7675c2717bd4796e4b0.

cortner commented 1 year ago

I think this is addressed by the PythonCall and MLJ extensions.