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

Fixes #4505 Remove reliance on getting product version for model.zip/version.txt from FileVersionInfo and replace with using assembly custom attributes #4512

Merged
merged 1 commit into from Oct 12, 2021

Conversation

r0ss88
Copy link
Contributor

@r0ss88 r0ss88 commented Nov 29, 2019

Refer to original issue for further details of the reasons for this change.

I have used AssemblyInformationalVersionAttribute to match the same behaviour as was introduced with the change to reliance of FileVersionInfo.GetVersionInfo. There is already a test covering -ModelFiles.DetermineNugetVersionFromModel in Microsoft.ML.FunctionalTests.

…from FileVersionInfo and replace with using assembly custom attributes (dotnet#4505)
@r0ss88 r0ss88 requested a review from a team as a code owner November 29, 2019 21:27
@dnfclas
Copy link

dnfclas commented Nov 29, 2019

CLA assistant check
All CLA requirements met.

@r0ss88
Copy link
Contributor Author

r0ss88 commented Dec 10, 2019

Any chance this could be PR'd? the change is pretty minor.

{
var assembly = typeof(RepositoryWriter).Assembly;

var assemblyInternationalVersionAttribute = assembly.CustomAttributes.FirstOrDefault(a =>

Choose a reason for hiding this comment

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

assemblyInternationalVersionAttribute [](start = 16, length = 37)

Nit: Can this be renamed assemblyInformationalVersionAttribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK sure


if (assemblyInternationalVersionAttribute == null)
{
throw new ApplicationException($"Cannot determine product version from assembly {assembly.FullName}.");

Choose a reason for hiding this comment

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

throw [](start = 16, length = 5)

Not sure we should throw in this case. Perhaps we should save 0.0.0.0 in this case? Or, alternatively, not include Version.txt in the model?

@eerhardt, @harishsk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was only to keep consistency with how it was behaving before - I prefer the approach you have suggested though it should not fail because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please confirm @eerhardt @harishsk @yaeldekel and I'll make the change.

@michaelgsharp
Copy link
Member

Hey @r0ss88, is still still something you would like to see merged in? You mention to refer to the original issue, but I don't see one linked. Which issue is it?

@michaelgsharp
Copy link
Member

Nevermind, I see it in the title not the description. Let me take a look at it real fast.

@michaelgsharp
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #4512 (4896fdd) into master (549b389) will decrease coverage by 6.61%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4512      +/-   ##
==========================================
- Coverage   74.91%   68.30%   -6.62%     
==========================================
  Files         908     1130     +222     
  Lines      160097   240132   +80035     
  Branches    17226    24921    +7695     
==========================================
+ Hits       119934   164011   +44077     
- Misses      35363    69664   +34301     
- Partials     4800     6457    +1657     
Flag Coverage Δ
Debug 68.30% <ø> (-6.62%) ⬇️
production 62.96% <ø> (-7.29%) ⬇️
test 89.13% <ø> (-1.15%) ⬇️

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

Impacted Files Coverage Δ
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs 0.00% <0.00%> (-100.00%) ⬇️
...oft.ML.Data/DataLoadSave/TrivialLoaderEstimator.cs 0.00% <0.00%> (-85.72%) ⬇️
...rosoft.ML.Data/DataLoadSave/CompositeDataLoader.cs 0.00% <0.00%> (-81.25%) ⬇️
...t.ML.Data/DataLoadSave/CompositeLoaderEstimator.cs 0.00% <0.00%> (-80.00%) ⬇️
test/Microsoft.ML.AutoML.Tests/Util.cs 50.00% <0.00%> (-50.00%) ⬇️
...rc/Microsoft.ML.TensorFlow/TensorTypeExtensions.cs 34.61% <0.00%> (-47.21%) ⬇️
...Microsoft.ML.Data/DataLoadSave/TransformWrapper.cs 23.52% <0.00%> (-38.24%) ⬇️
.../Microsoft.ML.Data/Model/ModelOperationsCatalog.cs 55.37% <0.00%> (-36.37%) ⬇️
...rosoft.ML.Data/DataView/CompositeRowToRowMapper.cs 43.54% <0.00%> (-29.18%) ⬇️
src/Microsoft.ML.Data/LoggingEventArgs.cs 73.33% <0.00%> (-26.67%) ⬇️
... and 739 more

@michaelgsharp
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@michaelgsharp
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@michaelgsharp michaelgsharp left a comment

Choose a reason for hiding this comment

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

Looks good and we need this. Not sure why it wasn't followed up on.

@michaelgsharp
Copy link
Member

. The only failing test is a stale badge that won't refresh. Merging anyways.

@michaelgsharp michaelgsharp merged commit 9b28b24 into dotnet:main Oct 12, 2021
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Mar 19, 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.

None yet

4 participants