Alphapeptdeep Integration #3123
Conversation
pwiz_tools/Skyline/packages.config
Outdated
| @@ -0,0 +1,4 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <packages> | |||
| <package id="OneOf" version="3.0.271" targetFramework="net472" /> | |||
There was a problem hiding this comment.
This works on my development machine, but the build on TeamCity seems to be failing because it does not see this dll.
So far we have been adding all of the dll's that we need to:
pwiz_tools/Shared/lib
There was a problem hiding this comment.
For modern Nuget we use PackageReference, like this for Ardia in the Skyline.csproj file:
<ItemGroup>
<PackageReference Include="Microsoft.Web.WebView2">
<Version>1.0.2420.47</Version>
</PackageReference>
</ItemGroup>
There was a problem hiding this comment.
Sounds good. Thanks for the suggestion here. I've updated OneOf to use PackageReference. It works locally. Let's see if the teamcity test can pass now
There was a problem hiding this comment.
Everything builds fine on TeamCity, but now it looks like "OneOf.dll" is not getting copied to where it needs to go.
The test "CheckForMissingResources" is failing to load "Skyline-daily.exe":
System.IO.FileNotFoundException: Could not load file or assembly 'OneOf, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. The system cannot find the file specified.
File name: 'OneOf, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'
Does anyone know how to fix something like this?
|
Something went wrong with the merge here. It shouldn't show changes from other PRs. Maybe try fetching latest master and rebasing your branch on it. Do you need help doing that?. |
My guess is that pushing "Update branch" will fix everything. |
xgwang-uw
force-pushed
the
Skyline/work/20240816_pythoninstaller_with_virtual_env_support
branch
from
868fde7
to
416c9d2
Compare
Yeah. Thanks for asking. I did a rebase which treated the updated changes and anything in between as new commits. I rolled back those changes and added the updates as a new commit. That fixed this branch. I will do a merge from master later on. |
xgwang-uw
force-pushed
the
Skyline/work/20240816_pythoninstaller_with_virtual_env_support
branch
3 times, most recently
from
3db9cca
to
b007485
Compare
|
On TeamCity the TestData test runs from an installed directory (installed with the WIX-based installer). Any non-vendor DLLs have to be added into |
…IResources into ToolsResources
xgwang-uw
force-pushed
the
Skyline/work/20240816_pythoninstaller_with_virtual_env_support
branch
from
a4971e8
to
7812cb5
Compare
…ry with alphapeptdeep" This reverts commit 5c487ab. Tempororily remove this commit to make TeamCity build succeeds
…om skyline document
…alphapeptdeep cli to build library
… library file to be skyline compatible
…to .blib and import into skyline
xgwang-uw
changed the title
| private string PeptdeepExecutablePath => Path.Combine(PythonVirtualEnvironmentScriptsDir, PEPTDEEP_EXECUTABLE); | ||
| private string RootDir => Path.Combine(ToolDescriptionHelpers.GetToolsDirectory(), ALPHAPEPTDEEP); | ||
| private string SettingsFilePath => Path.Combine(RootDir, SETTINGS_FILE_NAME); | ||
| private string InputFileName => INPUT + UNDERSCORE +Document.DocumentHash +EXT_TSV; |
There was a problem hiding this comment.
"Document.DocumentHash" might contain the character "/" which would cause problems here.
You could use "PathEx.ReplaceInvalidFilenameCharacters" to get rid of that.
It might make more sense to put all of the temporary files into a subfolder of the .blib file that is being created. That is, if the user is building "foo.blib" then all of the temporary files go into "foo.blib.temp".
We know that the user has write permissions on the folder where the .blib file is being created.
I am not sure whether we are guaranteed that the user will be able to write to the tools directory.
| { | ||
| // TODO(xgwang): update this exception to an Alphapeptdeep specific one | ||
| throw new Exception( | ||
| @$"Modification {mod} is missing unimod ID, which is required by AlphapeptdeepLibraryBuilder"); |
There was a problem hiding this comment.
ModifiedSequence.Modification does not override ToString(), so you should use {mod.Name} here instead of {mod}.
It would probably be better to keep track of all of the unsupported modifications that were found, and skip over those peptides, and build a library with as many peptides as possible, and at the end show the list of unsupported modifications which caused some peptides to be skipped.
| /// </summary> | ||
| private static readonly Dictionary<int, string> AlphapeptdeepModificationName = new Dictionary<int, string>() | ||
| { | ||
| {4, @"Carbamidomethyl@C"}, |
There was a problem hiding this comment.
In hindsight, these are going to need to be detected by mass and amino acid. It is possible to have these modifications without their being correctly identified by unimod ID.
| /// The peptdeep cmd-flow command is how we can pass arguments that will override the settings.yaml file. | ||
| /// This is how the peptdeep CLI supports command line arguments. | ||
| /// </summary> | ||
| private Dictionary<string, string> CmdFlowCommandArguments => |
There was a problem hiding this comment.
Dictionaries are inherently unordered. This would be better expressed with an IList<Tuple<string, string>> Or IList<KeyValuePair<string, string>>. Or often I recommend just creating a struct or class with a name and well named members to make the code as self documenting as possible: IList.
| } | ||
| } | ||
|
|
||
| private static string CreateDirIfNotExist(string dir) |
There was a problem hiding this comment.
This is redundant. CreateDirectory() already works this way.
| { | ||
| // TODO(xgwang): update this exception to an Alphapeptdeep specific one | ||
| throw new Exception( | ||
| @$"Modification {mod} is missing unimod ID, which is required by AlphapeptdeepLibraryBuilder"); |
There was a problem hiding this comment.
Exception text that will be shown to the user needs to be localizable. So, it will need to be put in a RESX file, which means you can't use @ and you can't use this kind of inline value expansion.
| { | ||
| // TODO(xgwang): update this exception to an Alphapeptdeep specific one | ||
| throw new Exception( | ||
| @$"Modification with unimod ID of {unimodId} is not yet supported by Alphapeptdeep. Please remove such modifications and try again."); |
There was a problem hiding this comment.
Needs to be in a RESX file.
| @@ -286,7 +297,7 @@ private bool ValidateBuilder(bool validateInputFiles) | |||
| // TODO: Probably need to validate that all the libraries can be loaded into memory with progress UI | |||
| } | |||
|
|
|||
| // TODO: Create CarafeLibraryBuilder class with everything necessary to build a library | |||
| // TODO: Create AlphapeptdeepLibraryBuilder class with everything necessary to build a library | |||
There was a problem hiding this comment.
Seems like the original text is still appropriate.
| @@ -132,7 +132,7 @@ | |||
| <value>1</value> | |||
| </data> | |||
| <metadata name="helpTip.TrayLocation" type="System.Drawing.Point, System.Drawing, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a"> | |||
| <value>155, 17</value> | |||
| <value>148, 17</value> | |||
There was a problem hiding this comment.
Again, unclear why the form should need to change at all for this PR.
| @@ -30,6 +30,7 @@ | |||
| <ResolveAssemblyWarnOrErrorOnTargetArchitectureMismatch>None</ResolveAssemblyWarnOrErrorOnTargetArchitectureMismatch> | |||
| <EnableSecurityDebugging>false</EnableSecurityDebugging> | |||
| <AutoGenerateBindingRedirects>false</AutoGenerateBindingRedirects> | |||
| <RuntimeIdentifier>win</RuntimeIdentifier> | |||
There was a problem hiding this comment.
Is this change necessary?
|
|
||
| namespace pwiz.Skyline.ToolsUI | ||
| { | ||
| public partial class PythonInstallerDlg : FormEx |
There was a problem hiding this comment.
We should just use MessageDlg or MultiButtonMessageDlg instead of this custom form. All it does now is show a message and buttons. There are other good examples of this for installation.
| } | ||
| namespace pwiz.Skyline.ToolsUI | ||
| { | ||
| partial class PythonInstallerLegacyDlg |
There was a problem hiding this comment.
Lots of whitespace changing in the files associated with PythonInstallerLegacyDlg. Turning off whitespace diffs the changes look much more like the simple rename expected.
PythonInstallertoPythonInstallerLegacyDlgPythonInstallerimplementation to support Python virtual environmentPythonInstallerimplementation