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

[WIP] rough cut improved F# support #279

Closed
wants to merge 10 commits into from
Closed

Conversation

dsyme
Copy link
Member

@dsyme dsyme commented Apr 21, 2016

This is a very early rough cut of improved F# support which utilizes the Roslyn-like FSharp.Compiler.Service compiler to do dynamic compilation.

Below is a screen grab of this in practice:

liftoff

for this function (the transliteration of the C# source)

let Run(req: HttpRequestMessage , log: TraceWriter ) : Task<HttpResponseMessage>  =
    let queryParamms = req.GetQueryNameValuePairs().ToDictionary((fun p -> p.Key), (fun p -> p.Value), StringComparer.OrdinalIgnoreCase)

    log.Verbose(sprintf "F# HTTP trigger function processed a request. Name=%A" req.RequestUri)

    let res =
        match queryParamms.TryGetValue("name") with 
        | true, name -> 
            new HttpResponseMessage(HttpStatusCode.OK,Content = new StringContent("Hello " + name))
        | _ -> 
            new HttpResponseMessage(HttpStatusCode.BadRequest, Content = new StringContent("Please pass a name on the query string"))

    Task.FromResult(res)

The execution time for a function goes down from >5sec to the same as C# (milliseconds).

The Roslyn-specific parts of the C# support is factored out into CSharpCompiler and CSharpCompilation. The F# support implements corresponding interfaces. This allows nearly all the resolution machinery for nuget packages and so on to be shared between the two settings.

There is a lot more to do but I thought I'd give early visibility to this work since there are various ways of doing this. Some outstanding issues are

  • In the F# programming model, out arguments should perhaps be returned in a tuple rather than out parameters. This would increase the work we have to do.
  • In the F# programming model, the methods should perhaps use F# async { ... } rather than tasks (the two are inter-convertible but async { ... } is more natural to the F# programmer)
  • I've taken a couple of liberties in the code, e.g. I can't quite work out where ScriptClassName (Submission#0) comes from and why we search the C# generated types for that name.

Some things we should clarify early

  • Is using FSharp.Compiler.Service appropriate?
  • Is factoring the C# support between a generic-.NET part and a C#-specific part appropriate?

@azurecla
Copy link

Hi @dsyme, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

@fabiocav
Copy link
Member

fabiocav commented Apr 21, 2016

@dsyme thank you for sending this! Really excited about improving the F# experience in Azure Functions!

I'm taking a closer look at the changes, but some of the things here like breaking the C# compilation logic out of the invoker (what you've done in your change with the CSharpCompiler and the CSharpCompilation) were things we had in progress, so I'll compare those to what you needed to do to make sure it provides the functionality you required here.

Addressing your questions/comments above:

I can't quite work out where ScriptClassName ( Submission#0 ) comes from...

This is the default Roslyn main scripting type. We use that name to get that type specifically and look for the entry point method within it. Methods defined in your C# function script are ultimately part of this type.

Is using FSharp.Compiler.Service appropriate?

This is what we were investigating :) So I think it is appropriate but would follow your guidance here.

Is factoring the C# support between a generic-.NET part and a C#-specific part appropriate?

If I understand your question correctly, the answer is yes. This was the plan in order to support additional .NET languages (F# included), so the plan was to refactor types like the CSharpFunctionInvoker, CSharpFunctionSignature and CSharpFunctionDescriptionProvider into common generic .NET types that would provide the base functionality required by managed languages.

There are also a few other changes to the invoker model we've been working on, but those are primarily to align the Node and other scripting invokers with the model used by C# today, so I wouldn't expect them to impact this work.

Again, we're very excited about your contribution here! Better F# support has been one of our top customer requests, so a lot of people will be very happy to see this done!

Please let me know if there's anything we can do to make this process easier or if you run into any issues as you move forward.

@fabiocav
Copy link
Member

@dsyme I had the compilation service abstraction changes out for your review, but what you have is very close to those changes, so to avoid creating more unnecessary work for you, I was wondering if it would be possible to break those changes out into their own PR as they would be beneficial out of the scope of F#. That's assuming you feel you still have a fair amount of work left on the F# implementation.

Thanks!

@dsyme
Copy link
Member Author

dsyme commented Apr 22, 2016

@fabiocav I think there's not too much more work to do. I need to look into testing now, and there seem to be issues with propagating diagnostics.

Could you send me a link to the changes you were making? I can deal with a merge if needs be,

Cheers
don

@dsyme
Copy link
Member Author

dsyme commented Apr 22, 2016

@fabiocav BTW it would be really great if we could get GitHub-integrated CI on this repo, so that the full tests are being run on each PR and commit. That would help give confidence that there are no regressions going on :)


MethodInfo entryPointReference = entryPointResolver.GetFunctionEntryPoint(methods).Value;

// For F#, this currently creates a malformed signautre with fewer parameter symbols than parameter names.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo: signature

@dsyme
Copy link
Member Author

dsyme commented Apr 22, 2016

@fabiocav BTW I'm not sure why it's saying CLA-required - I'm a Microsoft employee - AFAIK it should be automatically CLA-not-required?

@forki
Copy link
Contributor

forki commented Apr 22, 2016

@dsyme looks they have that painful old CLA process where you have to let your manager sign. That will take ages for me.

@dsyme
Copy link
Member Author

dsyme commented Apr 22, 2016

@fabiocav I've got a few questions about the reference model.

  1. It seems that some C# #r references are resolved from the set of DLLs that make up the Azure Functions implementation, e.g. #r "NewtonSoft.Json. Currently these aren't being resolved by F# since the F# Compiler Service doesn't have a notion of a "MetadataResolver" in the same way as Roslyn.

    We could in theory add that to FCS. However I have conceptual problem with relying on a MetadataResolver - in particular it is no longer simple to edit scripts in Visual Studio or another "normal" F# editing environment, since the editing environment doesn't process (type-check) the script with the appropriate resolver.

  2. Equally, I know you're adding support for nuget packages and project.json. Do you expect all references to go via that route going forward? Is C:\GitHub\dsyme\azure-webjobs-sdk-script\test\WebJobs.Script.Tests \TestScripts\CSharp\NotificationHubOutNotification\project.json a typical example? If so I will try to adjust the F# samples to use project.json and see where we get to. (I'd also be in favour of adding a hook to enable Paket support, but that's a slightly separate issue). Thanks

@davidebbo
Copy link
Contributor

The CLA process is the standard one for the whole Azure organization, and is pretty painless.

@forki
Copy link
Contributor

forki commented Apr 22, 2016

unfortunately it says I have to let my boss sign. That's not painless for me (and not painless for my boss). IIRC the visualfsharp repo got rid of that requirement a while ago. maybe @KevinRansom or @latkin can tell which lawyer they bribed.

@KevinRansom
Copy link

Disclaimer: I am not a Lawyer :-)

@davidebbo I think most of Devdiv OSS uses the Microsoft cla: https://cla.microsoft.com. You might want to evaluate whether the Microsoft CLA is sufficient for Azure OSS contributions. It has the benefit of not requiring current employers to sign on behalf of their OSS contributing employees.

@davidebbo
Copy link
Contributor

Sorry, I have no control over this process. It is not specific to this repo, but used widely over the huge Azure org.

@fabiocav
Copy link
Member

fabiocav commented Apr 22, 2016

@dsyme we can appropriately label the PR since it didn't recognize you. You may need to link MSFT and GitHub accounts here: https://azureopensource.azurewebsites.net/ so the azureclabot will recognize you in the future.

@isaacabraham
Copy link
Contributor

@dsyme this is a great step forward. Some things that spring to mind: -

  1. There's actually an issue in the main webjobs SDK repo regarding removing out parameters (Function return values instead of ICollector / IAsyncCollector azure-webjobs-sdk#656). It would certainly be nice having an F#-centric story without the need to do the usual Async.StartAsTask malarky and not having out parameters etc.
  2. Regarding references - in my (crude) early tests, I simply bundled up my dependencies and manually did #r on them - IIRC I had to do a check if it was in interactive mode on not but that was probably just that I was hacking things together. In a perfect world I would like to see packages restored in a sane manner that reflects what happens locally i.e. if you restore to ..\..\packages\ relative to the script, I would want the same to happen when executed there. This is my intention for Paket integration too.

@fabiocav
Copy link
Member

fabiocav commented Apr 22, 2016

@dsyme to answer your questions above:

  1. Yeah, I understand the concern. Our usage of the #r directive is consistent with the way Roslyn works (resolving framework assemblies or assemblies based on a path), but we do support a couple of additional features:

    • Simple name private assembly reference: You can place a private assembly in a bin folder inside of your function folder and just reference it by simple file name using #r "MyAssembly.dll". This is a convenience and something you may choose not to support with F#.
    • Shared assembly references: Some assemblies (e.g. Json.NET assembly, ASP.NET WebHooks SDK and receivers, Azure Storage, Service Bus, etc.) are commonly needed when working with functions, so to eliminate the requirement to add a package reference to pull them in (or deploy them as private assemblies), we've added the ability to reference those assemblies by simple name. You still have the ability to use specific versions using the other mechanisms mentioned, but the common scenario is covered in a way that eliminates friction.

    In addition to the above, we automatically inject referenced package assemblies and a few other assemblies are also added by default (see here).

  2. Yes, that is the model we expect to continue to follow for packages (for assemblies in general, the mechanisms above will continue to be supported). We'll likely have the appropriate extensibility points so you could plug in a different package management solution like Paket in the future, but project.json will be what we'll support _out of the box _, trying to maintain the experience consistent with what you'll find in VS, .NET Core, etc.
    For the package references, the runtime itself does not make any assumptions as to where those assemblies are located. We simply use the project.lock.json resulting from a NuGet package restore against a project.json for the function (by default, when hosted in Azure, the restore uses a special folder as the package directory under the site's data folder to make sure that is available across all instances)

This is pretty high level, just trying to give you a good overview of how things currently work, but we can dive into details from here when/if needed.

@isaacabraham
Copy link
Contributor

@fabiocav how are you going to handle updates to e.g. Newtonsoft.Json - how will developers know which version will be bundled for free?

@fabiocav
Copy link
Member

There are a couple of proposals on how we'll manage that, but we haven't settled on the approach yet, so I don't have much to share, but this is absolutely something we'll be addressing. The entire versioning story goes beyond shared assemblies, into the runtime, Web Jobs SDK, extensions, etc. We're actively working on that and will start sharing details as soon as we can.

@dsyme
Copy link
Member Author

dsyme commented Apr 23, 2016

@isaacabraham @fabiocav Quick question, basically unrelated but useful for my testing.

  • I've been trying to find examples of git repos containing some Azure Functions. Do you have any links to sample git repos?
  • When I create Functions manually via the online editor I've not been able to retrieve the created contents via Git. If I set up Git it then says those functions are read-only. I'd somehow expected to be able to get the contents via git and then push revised contents Am I missing something?

@dsyme
Copy link
Member Author

dsyme commented Apr 23, 2016

@davidebbo

so to eliminate the requirement to add a package reference to pull them in (or deploy them as private assemblies), we've added the ability to reference those assemblies by simple name.

It's off topic for this PR, but at first sight this sounds like it might be a false-efficiency. I get the feeling that you might not be doing the programmer any favors by making trying to make this simpler: they will just hit version conflicts and/or confusion later on.

Adopting an approach based purely on high-quality, version-consistent package management like project.json (and optionally Paket eventually) seems better.

(If the references are dictated by the fact they are in-process references utilized by the AF compilation/loading machinery then that's a slightly different matter, and in that case a different approach to isolation may be needed. http://mbrace.io and other cloud execution machinery had similar issues, generally solved by minimizing the work loading machinery and using app

Best
Don

@isaacabraham
Copy link
Contributor

isaacabraham commented Apr 23, 2016

@dsyme Here's a gist of what I was using for testing it out... https://gist.github.com/isaacabraham/24a376d3fb6ca882528e42ccf4a8a9e0. I actually just created a local git repo in the functions website and deployed from there - folders map to functions within the portal.

And yes I saw exactly the same thing regarding read-only - it's referring (I believe) to the broswer editor only. You can push changes via Git and it picks them up no problem.

@dsyme
Copy link
Member Author

dsyme commented Apr 23, 2016

@isaacabraham Thanks. Have you by any chance got an example of a whole repo? e.g. with private bin directory and so on. I was mainly trying to understand what is and isn't allowed in the directory structure of the repo.

@isaacabraham
Copy link
Contributor

Yeah, I'm just looking for it - it's somewhere on this machine! When I find it I'll pop it on GH.

@davidebbo
Copy link
Contributor

FYI, I have started a mail thread with @martinwoodward, @forki and @KevinRansom to discuss the CLA part.

@dsyme dsyme closed this May 6, 2016
@dsyme dsyme reopened this May 6, 2016
@msftclas
Copy link

msftclas commented May 6, 2016

Hi @dsyme, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Don Syme). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@dsyme
Copy link
Member Author

dsyme commented May 6, 2016

"Total tests: 130. Passed: 119. Failed: 11. Skipped: 0."

Great!!! That gives us something to work on next week :) cc @tpetricek

@tpetricek
Copy link
Contributor

Not a bad score for a rough cut :). I see we also successfully managed to break one Node test 👍

@fabiocav
Copy link
Member

fabiocav commented May 6, 2016

👍
@tpetricek, you get bonus points for breaking Node! (unfortunately (?), the Node test that failed is a known flaky test and it's unlikely it had anything to do with your changes, so keep trying :) )

@fabiocav
Copy link
Member

@dsyme , @tpetricek; just wanted to check in and see if you need any help with this. Let us know if there's anything we can do.

@dnfclas
Copy link

dnfclas commented Jul 16, 2016

Hi @dsyme, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dsyme
Copy link
Member Author

dsyme commented Jul 16, 2016

OK, I've merged the work of @fabiocav into the right branch and this PR is reinstated back to Work In Progress :)

new HttpResponseMessage(HttpStatusCode.BadRequest, Content = new StringContent("Please pass a name on the query string"))

return res
} |> Async.StartAsTask

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme it's possibile to add the Async.StartAsTask as another special case of the return type inside the host when the function is called, instead of inside the function itself?

c# does something like that to support async vs sync version of Run.

ref DotNetFunctionInvoker implementation

so it's possibile to write

let Run (req: HttpRequestMessage , log: TraceWriter)  = async {
    return "ciao"
}

@fabiocav
Copy link
Member

I have merged this into the fsharp branch (https://github.com/Azure/azure-webjobs-sdk-script/tree/fsharp) and made the necessary fixes to resolve some of the compilation and test issues.

There are a few tests failing https://ci.appveyor.com/project/appsvc/azure-webjobs-sdk-script-y8o14/build/1.0.10326 but we should now be able to iterate and submit PRs against that branch to get things going.

@fabiocav fabiocav closed this Jul 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet