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

Add interface point for specifying acceleration in a composite models #762

Closed
wants to merge 2 commits into from

Conversation

ablaom
Copy link
Member

@ablaom ablaom commented May 2, 2022

This PR:

  • introduces a mechanism for implementing accelerated training of composite models that have been defined by "manually" exporting a learning network
  • implements the mechanism for Stacks

An alternative approach with a different interface point for acceleration is provided by #759 .

Supposing the composite model struct has acceleration::AbstractResource as a hyper-parameter, one simply adds acceleration=acceleration in the learning network machine constructor. Here's an example:

mutable struct MyAverageTwo <: DeterministicComposite
    rgs1 # deterministic regressor
    rgs2 # deterministic regressor
    acceleration::AbstractResource
end

function MLJModelInterface.fit(model::MyAverageTwo, verbosity, X, y)

    X = source(X)
    y = source(y)

    m1 = machine(model.rgs1, X, y)
    y1 = predict(m1, X)

    m2 = machine(model.rgs2, X, y)
    y2 = predict(m2, X)

    yhat = 0.5*y1 + 0.5*y2

    # learning network machine
    mach = machine(Deterministic(), X, y; predict=yhat, acceleration=model.acceleration)

    return!(mach, model, verbosity)

end

To support resources other than CPU1(), requires new overloadings of fit!(::Node, acceleration; kwargs) as shown here for CPUThreads. In a separate draft branch, I have done just that, and successfully tested a Stack.

TODO:

  • Update documentation (for fit!(::Node) and fit!(:Machine{<:Surrogate} and fit!(::Machine).
  • Add test that acceleration=CPUThreads() in Stack throws expected unsupported acceleration error.
  • See if integration into @from_network is possible

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2022

Codecov Report

Merging #762 (4b8c65b) into dev (2b5f40a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #762      +/-   ##
==========================================
+ Coverage   85.80%   85.81%   +0.01%     
==========================================
  Files          36       36              
  Lines        3438     3441       +3     
==========================================
+ Hits         2950     2953       +3     
  Misses        488      488              
Impacted Files Coverage Δ
src/composition/learning_networks/machines.jl 91.95% <100.00%> (ø)
src/composition/models/stacking.jl 94.59% <100.00%> (+0.07%) ⬆️
src/machines.jl 86.80% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b5f40a...4b8c65b. Read the comment docs.

@ablaom ablaom mentioned this pull request May 3, 2022
4 tasks
@ablaom
Copy link
Member Author

ablaom commented May 3, 2022

@olivierlabayle

@ablaom ablaom marked this pull request as draft May 3, 2022 00:15
@olivierlabayle
Copy link
Collaborator

@ablaom Thanks, I've had a look at the PR, it's a bit different from what I expected. I originaly thought you would just add it as a model hyperparameter and pass the acceleration to the return! function. I am not sure I understand why acceleration is now a field of both Machine and CompositeModel types.

What would be the use case for a non-composite model? it seems to create multiple entrypoints for acceleration which I find a bit confusing.

@ablaom
Copy link
Member Author

ablaom commented May 5, 2022

it seems to create multiple entrypoints for acceleration

Not really, but I understand the confusion. I am frequently confusing the machine that wraps a composite model and concrete data (which has no "downstream" machines, because it's arguments are just Sources) with the machines in a the learning network that fit! generates under the hood (which generally does). Perhaps that's the source of confusion.

The main reason for adding acceleration as a field to Machine is as a way to get parallelism happening in models defined as shown above. The key modification is the re-implementation of fit!(Machine{<:Surrogate}) here. I have made an analogous modification here but that is not necessary to get the above behaviour, nor does it conflict with it. I added it because it seemed natural to do so, and means I can fit a machine within a learning network (an ordinary machine with Node arguments, not a learning network machine) and get parallelism by specifying fit!(mach, acceleration=...). I don't think this is an important use-case, because we're mainly interested in exporting learning networks as stand-alone types, and in that case, only the learning network (Surrogate) machine is fit!. So I'm open to just dumping this or at least not documenting the possibilities it affords, if you think it's confusing.

@ablaom
Copy link
Member Author

ablaom commented May 5, 2022

I originaly thought you would just add it as a model hyperparameter

It is added as a hyperparameter. The parameter could be passed on using return! instead of the Surrogate machine constructor, I hadn't looked so see if that works. Is there an advantage?

@olivierlabayle
Copy link
Collaborator

I originaly thought you would just add it as a model hyperparameter

It is added as a hyperparameter. The parameter could be passed on using return! instead of the Surrogate machine constructor, I hadn't looked so see if that works. Is there an advantage?

I think the advantage is that you don't have to overburden the Machine with a new field that is only relevant for the Surrogate which is itself an artefact of Composition. I am quite confident it would work too since I have made similar thing here to prioritize the Composite's acceleration specification.

@ablaom
Copy link
Member Author

ablaom commented May 9, 2022

I agree, that having that the acceleration interface point in return! is preferable to the implementation in this PR. It's simpler, and the extra generality afforded by this PR is not worth the possibility confusion. It will also be a little easier to integrate the change in @from_network. Let's continue our discussion of how to move forward at #759 .

@ablaom ablaom closed this May 9, 2022
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.

3 participants