-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Replace ConditionalFact usages with custom facts #2402
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
Replace ConditionalFact usages with custom facts #2402
Conversation
e5e4ecb
to
f013125
Compare
Ok, so it seems I stumbled upon an interesting problem here. It seems tests marked with Specifically, the failing Onnx tests are looking for the model files in the wrong directory. Debugging into it, this is the path is returned in the
This feels like some kind of shadow copy is going on here. I'm still looking into the tensorflow and MacOS fail. |
The TensorFlow failure seems to be of the same root cause - it was not running under full framework, now it runs. However, the directory
I have half a mind to make Could someone point me in the right direction, @tannergooding, @eerhardt? |
@Anipik has been looking in this same space. Adding him to comment. |
Yes. Also given the subject area (TensorFlow components) I would also invite @jignparm to possibly share his thoughts. |
I would prefer to disable these (failing onnx test) tests just on fullFramework, using notFullFrameworkFact or NotFullFrameworkAndx64. As most of the tests are passing using conditional Fact are passing on fullFramework. |
I am able to fix all the resnet failures in #2411 |
Awesome news on the test fixes, @Anipik. I'll wait until your PRs are merged and then rebase my branch onto the latest master. I did some small refactorings (e.g. bring the attribute class names in line and add |
Codecov Report
@@ Coverage Diff @@
## master #2402 +/- ##
==========================================
+ Coverage 71.24% 71.24% +<.01%
==========================================
Files 788 797 +9
Lines 141131 141162 +31
Branches 16115 16111 -4
==========================================
+ Hits 100545 100572 +27
- Misses 36121 36127 +6
+ Partials 4465 4463 -2
|
Now the Core 3.0 Debug intrinsics build failed. Looking into it, it tried to run the benchmarks in DEBUG build, which should not have happened - the |
bcc7bae
to
8d75ef1
Compare
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.
can you also remove this https://github.com/dotnet/machinelearning/blob/master/test/Directory.Build.props#L32 ?
test/Microsoft.ML.TestFramework/Attributes/EnvironmentSpecificTheoryAttribute.cs
Show resolved
Hide resolved
test/Microsoft.ML.TestFramework/Attributes/EnvironmentSpecificFactAttribute.cs
Show resolved
Hide resolved
@@ -1313,7 +1314,7 @@ public void EntryPointMulticlassPipelineEnsemble() | |||
} | |||
} | |||
|
|||
[ConditionalFact(typeof(BaseTestBaseline), nameof(BaseTestBaseline.LessThanNetCore30OrNotNetCore))] | |||
[LessThanNetCore30OrNotNetCoreFact("netcore3.0 output differs from Baseline")] |
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.
netcore3.0
=> netcoreapp3.0
. We should use the real TFM here, so we don't confuse people.
Repeated for all uses.
@eerhardt I tried to debug the benchmarkTheory failure. For some reason Debug Constant is not working in this PR. I checked the Binlog and it is getting defined.
I tried adding it to the project as well but it didn't work. any suggestions why ? |
The issue with this is that that particular nuget also contains the definition of [TestCategory(Cat)]
[Fact(Skip = "Need CoreTLC specific baseline update")]
public void CommandCrossValidation()
{ ... } would become [Trait("Category", Cat)]
[Fact(Skip = "Need CoreTLC specific baseline update")]
public void CommandCrossValidation()
{ ... } Alternatively, we could use the [Category(Cat)]
[Fact(Skip = "Need CoreTLC specific baseline update")]
public void CommandCrossValidation()
{ ... } This nuget also contains some other useful attributes, like |
The Looking at the
That will completely override anything that is set earlier in a <DefineConstants>$(DefineConstants);CORECLR</DefineConstants> But even better would be if we deleted this line from the |
Sounds good. I'll take a look at this over the weekend, see what falls out. To clarify, we are talking specifically the |
Yes. The |
Add AttributeUsage attributes
b1c8011
to
bc7b87b
Compare
@@ -1096,23 +1096,6 @@ protected Func<bool> GetColumnComparer(Row r1, Row r2, int col, ColumnType type, | |||
return GetComparerVec<RowId>(r1, r2, col, size, (x, y) => x.Equals(y)); | |||
} | |||
|
|||
#if !CORECLR // REVIEW: Port Picture type to CoreTLC. | |||
if (type is PictureType) |
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 think this could have been ported from PictureType
to ImageType
, if I understand the problem correctly? Would require a reference to ImageAnalytics
, though.
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.
Currently we are only building the project with CORECLR
defined. So this is dead code that can be deleted.
Let's just delete it for now. If @Ivanidzo4ka thinks this is important, we can open a new issue to add image support here. Thoughts @Ivanidzo4ka?
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.
Feel free to delete. Some legacy code which we don't care about anymore
/// <summary> | ||
/// A fact for tests requiring full .NET Framework. | ||
/// </summary> | ||
public sealed class NotFullFrameworkFactAttribute : EnvironmentSpecificFactAttribute |
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.
Is this used? I don't see any usages doing a simple search.
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.
Just one comment on a class that doesn't appear to be used. Other than that, this looks good. Thanks for tackling this - I think it really cleans the tests up nicely.
Got rid of the unused attribute. Glad to be of help 😄 |
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.
LGTM
As proposed in #1220, this PR replaces all usages of
ConditionalFact
in tests with custom-tailored, environment-specific facts.These specific facts all inherit from a single base
EnvironmentSpecificFactAttribute
. Several of the facts check the same condition (e.g. TensorFlow, Onnx, and X64 - they all simply check for X64 process) but I decided not to merge them - they might check the same thing now but I expect they might branch out later on. I consider the small duplication of code not worth the mental overhead of inheritance or composition here.Together with this I also removed some now-obsolete checking of platform in Onnx-related tests (they were all Windows-only anyway) and removed the old
OnnxFact
.Fixes #1220.