xamarin / xamarin-macios

.NET for iOS, Mac Catalyst, macOS, and tvOS provide open-source bindings of the Apple SDKs for use with .NET managed languages such as C#
Other
2.44k stars 507 forks source link

CoreML C# Binding Generator uses wrong protection level #3718

Open b-straub opened 6 years ago

b-straub commented 6 years ago

Steps to Reproduce

  1. Create a Xamarin.Mac application
  2. Add a CoreML model e.g. Google Places 365
  3. Use the model e.g. with Vision framework

Expected Behavior

The application will compile.

Actual Behavior

The application has a protection level compile error

Environment

var modelRaw = new googlenet_places365();
var modelVision = VNCoreMLModel.FromMLModel(modelRaw.model, out vnErr);

Build Logs

_ViewController.cs(32,57,32,62): error CS0122: 'googlenetplaces365.model' is inaccessible due to its protection level

Fix

The CoreML binding generator needs to insert public before the model property


public readonly MLModel model;
VincentDondain commented 6 years ago

Hi,

May I suggest a different approach to get the MLModel:

var assetPath = NSBundle.MainBundle.GetUrlForResource ("GooglePlaces365", "mlmodelc");
var mlModel = MLModel.Create (assetPath, out NSError mlErr);
var model = VNCoreMLModel.FromMLModel (mlModel, out NSError vnErr);

As far as I know, this is the way to create and use the CoreML model (see our documentation and this sample). I believe, this creation mechanism justifies the fact readonly MLModel model is private.

Feel free to re-open this issue if my answer didn't solve you problem (:

b-straub commented 6 years ago

Your approach duplicates code from the generated binding and would render the parameterless constructor of the generated main class fairly useless.

VincentDondain commented 6 years ago

Ah yes, on my first read of the generated code I thought Create was meant to be the only object creation mechanism but you are right the default constructor can also be used. If it is used, it means we need to be able to access the model it creates. Therefore we need to make it:

public readonly MLModel Model;

Compared to the generated Swift code, our access modifier is incorrect. In Swift all entities have a default access level of internal.


Edit (just figured the Swift generated code also let users construct a model with explicit path to mlmodel file)

@jstedfast can you explain why we have a static Create in addition to the default constructor?

public static MarsHabitatPricer Create (NSUrl url, out NSError error)

It seems to me that it's a redundant and "suboptimal" creation mechanism. Because the code is generated, the default generated constructor should always be able to load the model with the right name.


We do not quite support renaming the model file like Xcode does. When renaming the model file in VSMac we should re-generate the dependent cs file, update all the generated types for the new model name and finally update the model name passed to NSBundle.MainBundle.GetUrlForResource.

VincentDondain commented 6 years ago

For reference, here's the generated code for the MarsHabitatPricer model.

/// <summary>
/// Class for model loading and prediction
/// </summary>
public class MarsHabitatPricer : NSObject
{
    public readonly MLModel Model;

    public MarsHabitatPricer ()
    {
        var url = NSBundle.MainBundle.GetUrlForResource ("MarsHabitatPricer", "mlmodelc");
        NSError err;

        Model = MLModel.Create (url, out err);
    }

    MarsHabitatPricer (MLModel model)
    {
        this.Model = model;
    }

    public static MarsHabitatPricer Create (NSUrl url, out NSError error)
    {
        if (url == null)
            throw new ArgumentNullException (nameof (url));

        var model = MLModel.Create (url, out error);

        if (model == null)
            return null;

        return new MarsHabitatPricer (model);
    }

    /// <summary>
    /// Make a prediction using the standard interface
    /// </summary>
    /// <param name="input">an instance of MarsHabitatPricerInput to predict from</param>
    /// <param name="error">If an error occurs, upon return contains an NSError object that describes the problem.</param>
    public MarsHabitatPricerOutput GetPrediction (MarsHabitatPricerInput input, out NSError error)
    {
        var prediction = Model.GetPrediction (input, out error);

        if (prediction == null)
            return null;

        var priceValue = prediction.GetFeatureValue ("price").DoubleValue;

        return new MarsHabitatPricerOutput (priceValue);
    }

    /// <summary>
    /// Make a prediction using the convenience interface
    /// </summary>
    /// <param name="solarPanels">Number of solar panels as double</param>
    /// <param name="greenhouses">Number of greenhouses as double</param>
    /// <param name="size">Size in acres as double</param>
    /// <param name="error">If an error occurs, upon return contains an NSError object that describes the problem.</param>
    public MarsHabitatPricerOutput GetPrediction (double solarPanels, double greenhouses, double size, out NSError error)
    {
        var input = new MarsHabitatPricerInput (solarPanels, greenhouses, size);

        return GetPrediction (input, out error);
    }
}

Here's the generated swift code (default access level is internal):

/// Class for model loading and prediction
@available(macOS 10.13, iOS 11.0, tvOS 11.0, watchOS 4.0, *)
class MarsHabitatPricerRenamed {
    var model: MLModel

    /**
        Construct a model with explicit path to mlmodel file
        - parameters:
           - url: the file url of the model
           - throws: an NSError object that describes the problem
    */
    init(contentsOf url: URL) throws {
        self.model = try MLModel(contentsOf: url)
    }

    /// Construct a model that automatically loads the model from the app's bundle
    convenience init() {
        let bundle = Bundle(for: MarsHabitatPricerRenamed.self)
        let assetPath = bundle.url(forResource: "MarsHabitatPricerRenamed", withExtension:"mlmodelc")
        try! self.init(contentsOf: assetPath!)
    }

    /**
        Make a prediction using the structured interface
        - parameters:
           - input: the input to the prediction as MarsHabitatPricerRenamedInput
        - throws: an NSError object that describes the problem
        - returns: the result of the prediction as MarsHabitatPricerRenamedOutput
    */
    func prediction(input: MarsHabitatPricerRenamedInput) throws -> MarsHabitatPricerRenamedOutput {
        let outFeatures = try model.prediction(from: input)
        let result = MarsHabitatPricerRenamedOutput(price: outFeatures.featureValue(for: "price")!.doubleValue)
        return result
    }

    /**
        Make a prediction using the convenience interface
        - parameters:
            - solarPanels: Number of solar panels as double value
            - greenhouses: Number of greenhouses as double value
            - size: Size in acres as double value
        - throws: an NSError object that describes the problem
        - returns: the result of the prediction as MarsHabitatPricerRenamedOutput
    */
    func prediction(solarPanels: Double, greenhouses: Double, size: Double) throws -> MarsHabitatPricerRenamedOutput {
        let input_ = MarsHabitatPricerRenamedInput(solarPanels: solarPanels, greenhouses: greenhouses, size: size)
        return try self.prediction(input: input_)
    }
}