-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Nuget package updates #2690
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
Nuget package updates #2690
Conversation
You either need to include it to Microsoft.ML.Sweep nuget, or remove reference to it. Refers to: src/Microsoft.ML.Sweeper/Microsoft.ML.Sweeper.csproj:15 in bc31bb5. [](commit_id = bc31bb5, deletion_comment = False) |
@@ -3,12 +3,11 @@ | |||
<PropertyGroup> | |||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
<DefineConstants>CORECLR</DefineConstants> | |||
<IncludeInPackage>Microsoft.ML</IncludeInPackage> |
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.
ML [](start = 32, length = 2)
Microsoft.ML.Sweep? you need this dll for ResultProcessor.
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 would prefer to not put these in Sweep. Maybe another nuget package for execution? Microsoft.ML.Exec? This would include both maml and results processor.
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 if we just don't ship Microsoft.ML.Sweeper
in any NuGet package for now, until someone makes a case that it must be shipped?
In reply to: 259190210 [](ancestors = 259190210)
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.
BTW - if there is a high level name for a package that can include sweep, results processor and maml, Im open to that as well -- preferably its more directed and not something like "Microsoft.ML.Misc" as that I think leads to a catch-all situation.
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 double that, I don't see any reason to ship Sweeper right now.
I've went through code, and I don't see any entrypoints, so it's not consumable by Nimbus, and everything else we don't care right now.
Sorry for assigning you confusing issue.
In reply to: 259440526 [](ancestors = 259440526,259190210)
Codecov Report
@@ Coverage Diff @@
## master #2690 +/- ##
=========================================
Coverage ? 71.59%
=========================================
Files ? 805
Lines ? 142004
Branches ? 16119
=========================================
Hits ? 101673
Misses ? 35891
Partials ? 4440
|
@@ -6,7 +6,7 @@ | |||
</PropertyGroup> | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="..\Microsoft.ML.OnnxTransform\Microsoft.ML.OnnxTransform.nupkgproj" /> | |||
<ProjectReference Include="..\Microsoft.ML.OnnxTransformer\Microsoft.ML.OnnxTransformer.nupkgproj" /> |
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 is going on here?
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.
Uggh, I think nuget just skips over project references it can't find.
Can we get this fix in a separate PR?
In reply to: 259437310 [](ancestors = 259437310)
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.
Correct - you get just a warning. Sure I can move this into a different PR.
bc31bb5
to
bdeb162
Compare
OK, I removed ResultProcess, Maml and Sweeper from the Microsoft.ML package. I will also post a new PR that fixes the pathing for Onnx transform |
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.
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!
currently not in a nuget package
for ONNX Transformer.
This fixes #689