-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Smoke test for F# #600
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
Smoke test for F# #600
Conversation
I got this error initially on OSX
This is due to an F# compiler issue when using In the meantime we work around by not using |
This is ready for review etc. Felt like a lot of work for +196 lines. But still, there it is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work, @dsyme. This is good that we are getting F# test coverage in our repo.
In order to get your new test running, you'll need to add a line here:
machinelearning/test/run-tests.proj
Line 5 in dcc8ae8
<Project Include="$(MSBuildThisFileDirectory)**\*.csproj" /> |
<PropertyGroup> | ||
<TargetFrameworks>netcoreapp2.0</TargetFrameworks> | ||
<NoWarn>2003;$(NoWarn)</NoWarn> | ||
<TargetFrameworks Condition="'$(OS)' != 'Unix'">$(TargetFrameworks); net461</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) I'd keep this line next to the other TargetFrameworks
declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.7.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 PackageReferences are not needed, since all projects under the test
directory automatically include them.
machinelearning/test/Directory.Build.props
Lines 24 to 26 in dcc8ae8
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.5.0" /> | |
<PackageReference Include="xunit" Version="2.3.1" /> | |
<PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks!
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.7.0" /> | ||
<PackageReference Include="xunit" Version="2.3.1" /> | ||
<PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" /> | ||
<DotNetCliToolReference Include="dotnet-xunit" Version="2.3.1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed all together, since it is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okey dokey
[<Fact>] | ||
let ``FSharp-Sentiment-Smoke-Test`` () = | ||
|
||
let testDataPath = __SOURCE_DIRECTORY__ + @"/../data/wikipedia-detox-250-line-data.tsv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does __SOURCE_DIRECTORY__
work in non-interactive mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it does
<TargetFrameworks Condition="'$(OS)' != 'Unix'">$(TargetFrameworks); net461</TargetFrameworks> | ||
<NoWarn>2003;$(NoWarn)</NoWarn> | ||
<PublicSign>false</PublicSign> | ||
<SourceLink></SourceLink> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for? Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just got a build error when building from the IDE using VS 15.7 because for .NET 4.6.1 compilation "full" PDB symbols (not portable) were being produced), I can reproduce if I build with msbuild
not dotnet
. Full PDB are not compatible with sourcelink. It doesn't go away even if you set DebugType to "portable".
I think it is an F# build targets bug fixed in 15.8. Anyway for now seemed sensible to turn of SourceLink for the test project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, test is failing now it is running, oddly it's not failing on my machine. Will look into it
At least that's proof that it's running |
@eerhardt Is there any ongoing discussion about the design of ML.NET's Component Catalog mechanism, e.g. is there any prospect of scrapping it in favour of more traditional direct static library references? What's the driving motivation of the mechanism? I find it strange that we would have this extra level of indirection and it seems to be causing problems for F# - in scripts we need the artificial stuff mentioned in #401. The need for that kind of thing is always indicative of a problem (over over-complexification) in the underlying framework - "normal" .NET libraries don't need this sort of thing. Also, the component catalog seems to be populated via the assemblies reachable via direct static references (I'm not totally sure about that, but that's kind of the point - the mechanism is just obscure). It's too easily possible to write code where there there isn't a static reference to, say Basically, what is the component catalog really for, and is it really necessary? It's always going to sit very awkwardly alongside other .NET componentization models (including the traditional one of static references) - e.g. using it with DI frameworks might be really rough. Why isn't ML.NET just a normal set of .NET libraries using normal static assembly references? |
#208 is probably closest one. But it doesn't answer question why we have it in first place. I can give you one example where existence of component catalog make sense (at least for me). Simple scoring. With current code, you just do
and all you need to do is add nuget package and it will work. (at least that was the case for .net framework 4.5 where content of nuget files was put into you binary folder, in net standard we need to make changes in our component catalog) This way you don't have to add references on libraries in your project. In reply to: 409231443 [](ancestors = 409231443) |
Yes, there has been discussion here: #371. Basically, the plan of record is that we will move to directly calling the underlying C# APIs without going through the extra layer of indirection. That will solve the majority of the issues mentioned in #401. (Note, we have the same issue in Azure Functions - #559) However, that doesn't completely solve the issue of dependency injection. The other place where is must be used is when we load a So, in this scenario, we are still going to need dependency injection at runtime. That's the main usage scenario for component catalog. (It is also used by other tooling, like the MAML command line tool, and the Entry Points subsystem.) In investigating the Azure Functions issue, we discovered other issues with the way that the component catalog loads assemblies. At its heart, the issue is that we don't work well with custom AssemblyLoadContext instances. That issue is being tracked at #569. We will also have to change how the code that only looks in the current folder for components. And instead, we should probably be using the |
Was integrated in #616 |
This addresses the first part of #540, i.e. adding an initial smoke test for ML.NET and F#
Submitting now to check that CI passes ok, will then iterate.