Skip to content

Conversation

@ericstj
Copy link
Member

@ericstj ericstj commented Oct 18, 2022

#6385 and #6367 clashed after merge. Fix the build break.

new WebClient().DownloadFile(url, relativeFilePath);
using (var client = new HttpClient())
{
var response = await client.GetAsync(url);
Copy link
Member

Choose a reason for hiding this comment

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

response

do we need to dispose this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I copied this from elsewhere in the repo which doesn’t dispose. Perhaps disposing the wrapping client takes care of any open resources. I will look into it and follow up with all the instances of the problem in the repo if necessary.


https://github.com/dotnet/machinelearning/search?q=httpclient+getasync&type=

@ericstj
Copy link
Member Author

ericstj commented Oct 18, 2022

Unix failure is System.Drawing:

   Microsoft.ML.Tests.ImageTests.TestBackAndForthConversionWithAlphaInterleave [FAIL]
�[31;1m�[m�[37m      System.ArgumentException : Parameter is not valid.
�[m�[30;1m      Stack Trace:
�[m�[37m           at System.Drawing.SafeNativeMethods.Gdip.CheckStatus(Int32 status)
�[m�[37m           at System.Drawing.Bitmap.UnlockBits(BitmapData bitmapdata)
�[m�[37mStarting test: Microsoft.ML.Tests.ImageTests.TestSaveImages
        /__w/1/s/src/Microsoft.ML.ImageAnalytics/VectorToImageTransform.cs(421,0): at Microsoft.ML.Transforms.Image.VectorToImageConvertingTransformer.Mapper.<>c__DisplayClass5_0`1.<GetterFromType>b__0(Bitmap& dst)
�[m�[37m        /__w/1/s/test/Microsoft.ML.Tests/ImagesTests.cs(345,0): at Microsoft.ML.Tests.ImageTests.TestBackAndForthConversionWithAlphaInterleave()

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Merging #6388 (f7ce04e) into main (c8b3ca4) will increase coverage by 0.00%.
The diff coverage is 91.66%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6388   +/-   ##
=======================================
  Coverage   68.50%   68.50%           
=======================================
  Files        1165     1165           
  Lines      246202   246211    +9     
  Branches    25744    25744           
=======================================
+ Hits       168651   168667   +16     
+ Misses      70866    70860    -6     
+ Partials     6685     6684    -1     
Flag Coverage Δ
Debug 68.50% <91.66%> (+<0.01%) ⬆️
production 62.92% <0.00%> (+<0.01%) ⬆️
test 88.98% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...a.Analysis.Interactive/DataFrameKernelExtension.cs 90.83% <0.00%> (ø)
...rc/Microsoft.ML.SearchSpace/Option/ChoiceOption.cs 97.56% <ø> (ø)
src/Microsoft.ML.SearchSpace/Option/NestOption.cs 56.16% <ø> (ø)
...soft.ML.SearchSpace/Option/UniformNumericOption.cs 97.46% <ø> (ø)
src/Microsoft.ML.SearchSpace/Parameter.cs 74.05% <ø> (ø)
src/Microsoft.ML.SearchSpace/SearchSpace.cs 73.36% <ø> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs 97.29% <100.00%> (-0.55%) ⬇️
src/Microsoft.ML.FastTree/Training/StepSearch.cs 57.42% <0.00%> (-4.96%) ⬇️
...StandardTrainers/Standard/LinearModelParameters.cs 65.82% <0.00%> (-0.26%) ⬇️
... and 4 more

@ericstj
Copy link
Member Author

ericstj commented Oct 19, 2022

Definitely broke Linux, it’s hanging. Will see what I did wrong. Don’t merge please.

@tarekgh
Copy link
Member

tarekgh commented Oct 19, 2022

Could be .Wait() call? using await worth to try.

@ericstj
Copy link
Member Author

ericstj commented Oct 19, 2022

Can’t await unless I change the whole stack. ConfigureAwait is the solution.

@ericstj ericstj merged commit 60db8e8 into dotnet:main Oct 19, 2022
ericstj added a commit to ericstj/machinelearning that referenced this pull request Oct 19, 2022
ericstj added a commit to ericstj/machinelearning that referenced this pull request Oct 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants