-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Enable TensorFlowTransform to work with pre-trained models that are not frozen #853
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
Changes from 45 commits
1007955
40fbedc
48d14c6
35ff43a
6291d0d
57508d3
cfcd70f
236de73
781cff0
07b15a0
47f75b5
c304257
d0430b5
950a210
97eb497
173729f
292140b
655a8aa
be5285a
46c04a3
e705f93
84214e2
04d02b8
89693bd
8c8d92e
d8edc64
eea524e
b609ffd
74b8899
3382a83
aa8e844
25b1e64
e32acca
ac45539
ce4efef
8b8764b
f955488
6e11f2c
ed71513
7df343d
f883d78
ae672d6
fac8dae
2b1a576
21879f6
a1d912d
f6a1c84
5957c53
5120bb9
a624a3b
8d9fdc5
685ab99
2d0ec1e
8d8b986
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,8 @@ | |
using Microsoft.ML.Runtime.Data; | ||
using Microsoft.ML.Runtime.ImageAnalytics.EntryPoints; | ||
using Microsoft.ML.Runtime.Internal.Utilities; | ||
using System.Security.Principal; | ||
using System.Security.AccessControl; | ||
|
||
namespace Microsoft.ML.Transforms.TensorFlow | ||
{ | ||
|
@@ -158,6 +160,101 @@ internal static TFSession LoadTFSession(IExceptionContext ectx, byte[] modelByte | |
return new TFSession(graph); | ||
} | ||
|
||
private static TFSession LoadTFSession(IHostEnvironment env, string exportDirSavedModel) | ||
{ | ||
env.CheckValue(exportDirSavedModel, nameof(exportDirSavedModel)); | ||
var sessionOptions = new TFSessionOptions(); | ||
var exportDir = exportDirSavedModel; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
not needed. #Resolved |
||
var tags = new string[] { "serve" }; | ||
var graph = new TFGraph(); | ||
var metaGraphDef = new TFBuffer(); | ||
|
||
return TFSession.FromSavedModel(sessionOptions, null, exportDir, tags, graph, metaGraphDef); | ||
} | ||
// A TensorFlow frozen model is a single file. An un-frozen (SavedModel) on the other hand has a well-defined folder structure. | ||
// Given a modelPath, this utility method determines if we should treat it as a SavedModel or not | ||
internal static bool IsSavedModel(string modelPath) | ||
{ | ||
FileAttributes attr = File.GetAttributes(modelPath); | ||
return attr.HasFlag(FileAttributes.Directory); | ||
} | ||
|
||
internal static void CreateTempDirectory(string tempDirPath) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What's the main reason for this? Why we treat Windows differently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (replied on other comment about its necessity) Currently used in TFTransform only to protect SavedModels which are considered executable code by .NET security team In reply to: 219577130 [](ancestors = 219577130) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put comment with description why we doing this? In reply to: 219578542 [](ancestors = 219578542,219577130) |
||
{ | ||
//if directory exists, do nothing. | ||
if (Directory.Exists(tempDirPath)) | ||
return; | ||
|
||
WindowsIdentity currentIdentity = null; | ||
try | ||
{ | ||
currentIdentity = WindowsIdentity.GetCurrent(); | ||
} | ||
catch (PlatformNotSupportedException) | ||
{ } | ||
|
||
if (currentIdentity != null && new WindowsPrincipal(currentIdentity).IsInRole(WindowsBuiltInRole.Administrator)) | ||
{ | ||
// Create high integrity dir and set no delete policy for all files under the directory. | ||
// In case of failure, throw exception. | ||
CreateTempDirectoryWithAcl(tempDirPath, currentIdentity.User.ToString()); | ||
} | ||
else | ||
Directory.CreateDirectory(tempDirPath); | ||
} | ||
|
||
private static void CreateTempDirectoryWithAcl(string dirPath, string identity) | ||
{ | ||
// Dacl Sddl string: | ||
// D: Dacl type | ||
// D; Deny access | ||
// OI; Object inherit ace | ||
// SD; Standard delete function | ||
// wIdentity.User Sid of the given user. | ||
// A; Allow access | ||
// OICI; Object inherit, container inherit | ||
// FA File access | ||
// BA Built-in administrators | ||
// S: Sacl type | ||
// ML;; Mandatory Label | ||
// NW;;; No write policy | ||
// HI High integrity processes only | ||
string sddl = "D:(D;OI;SD;;;" + identity + ")(A;OICI;FA;;;BA)S:(ML;OI;NW;;;HI)"; | ||
|
||
var dir = Directory.CreateDirectory(dirPath); | ||
DirectorySecurity dirSec = new DirectorySecurity(); | ||
dirSec.SetSecurityDescriptorSddlForm(sddl); | ||
dirSec.SetAccessRuleProtection(true, false); // disable inheritance | ||
dir.SetAccessControl(dirSec); | ||
|
||
// Cleaning out the directory, in case someone managed to sneak in between creation and setting ACL. | ||
DirectoryInfo dirInfo = new DirectoryInfo(dirPath); | ||
foreach (FileInfo file in dirInfo.GetFiles()) | ||
{ | ||
file.Delete(); | ||
} | ||
foreach (DirectoryInfo subDirInfo in dirInfo.GetDirectories()) | ||
{ | ||
subDirInfo.Delete(true); | ||
} | ||
} | ||
|
||
internal static TFSession GetSession(IHostEnvironment env, string modelPath) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
nit: Add an empty line between methods. #Closed |
||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Contracts.Check(env, nameof(env); #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added this check. btw i believe you meant Contracts.Check(env != null, nameof(env)); In reply to: 220031028 [](ancestors = 220031028) |
||
if (IsSavedModel(modelPath)) | ||
return LoadTFSession(env, modelPath); | ||
|
||
return CheckFileAndRead(env, modelPath); | ||
} | ||
|
||
private static TFSession CheckFileAndRead(IHostEnvironment env, string modelFile) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
can you get rid of this method? no one using it any more except GetSession, and I think it would be much better to move checks in start of GetSesssion #Resolved |
||
{ | ||
env.CheckNonWhiteSpace(modelFile, nameof(modelFile)); | ||
env.CheckUserArg(File.Exists(modelFile), nameof(modelFile)); | ||
var bytes = File.ReadAllBytes(modelFile); | ||
return LoadTFSession(env, bytes, modelFile); | ||
} | ||
|
||
internal static unsafe void FetchData<T>(IntPtr data, T[] result) | ||
{ | ||
var size = result.Length; | ||
|
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.
A) Do we really need them? Because presence of Principal.Windows, for multiplatform code is suspicious.
B) If we really need them you need to make them part of nuget dependency, and also we need to modify https://github.com/dotnet/machinelearning/blob/master/build/Dependencies.props instead of specify versions here. #Resolved
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.
We use this as a security measure on windows platform. We are using temp directory for loading TFSession for SavedModels. Models are considered executable code, so for this temp directory we need to ACL it in the high-rights process so low-rights process can’t access it.
I will look into making it part of nuget dependency
In reply to: 219572600 [](ancestors = 219572600)
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.
you can use this one as reference implementation https://github.com/dotnet/machinelearning/blob/master/pkg/Microsoft.ML.ImageAnalytics/Microsoft.ML.ImageAnalytics.nupkgproj
In reply to: 219577320 [](ancestors = 219577320,219572600)