Alphapeptdeep Integration #3123

xgwang-uw · 2024-08-16T08:56:19Z

Rename existing PythonInstaller to PythonInstallerLegacyDlg
Add new PythonInstaller implementation to support Python virtual environment
Add functional tests for new PythonInstaller implementation

pwiz_tools/Skyline/packages.config

chambm · 2024-08-20T19:03:53Z

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?.

nickshulman · 2024-08-20T19:31:42Z

Something went wrong with the merge here.

My guess is that pushing "Update branch" will fix everything.
This branch has changes from master but does not know that it is up to date with master.
If we push the "Update branch" button it will notice all the files that it wants to change are identical to what is already there and everything will be good.

xgwang-uw · 2024-08-20T19:46:06Z

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?.

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.

chambm · 2024-08-22T16:15:12Z

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 Product-template.wxs manually. We didn't hit this on the Ardia PR because the Ardia tests are in TestConnected, which doesn't run under the admin-installed instance. Nick, we have both hit this snag many times over the years and it's funny how easily we forget it. :) I think I once proposed grepping for DLLs in the csproj files in the past but never got around to implementing it.

…IResources into ToolsResources

…alphapeptdeep

…ry with alphapeptdeep" This reverts commit 5c487ab. Tempororily remove this commit to make TeamCity build succeeds

…ilder class

…om skyline document

…alphapeptdeep cli to build library

… library file to be skyline compatible

… during testing. Also changes to the PythonTestUtil related to order of Message Dialogs presented during Python Install and Nvidia configuration.

…setup. Implement local functions for labeling precursors to address build message : WARNING: Found prohibited use of "using.*(pwiz\.Skyline\.(Alerts|Controls|.*UI)|System\.Windows\.Forms|pwiz\.Common\.GUI)" (Skyline model code must not depend on UI code) at C:\pwiz\pwiz_tools\Skyline\Model\AlphaPeptDeep\AlphapeptdeepLibraryBuilder.cs(14) using pwiz.Skyline.Controls.SeqNode;

…rk/20240816_pythoninstaller_with_virtual_env_support

…pend on UI code'

…to PythonInstaller revealed by BPratt's buddy testing

…rk/20240816_pythoninstaller_with_virtual_env_support

…yline/work/20240816_pythoninstaller_with_virtual_env_support and address issues brought up during buddy testing # Resolved Conflicts: # pwiz_tools/Skyline/SettingsUI/PeptideSettingsUI.cs

bspratt · 2025-01-31T20:12:30Z

pwiz_tools/Skyline/TestUtil/PythonTestUtil.cs

@@ -10,7 +29,7 @@
 namespace pwiz.SkylineTestUtil
 {
    [TestClass]


Drop this - this isn't a test, it's code supporting tests

bspratt · 2025-01-31T20:17:01Z

pwiz_tools/Skyline/TestUtil/PythonTestUtil.cs

        private void RunNvidiaDialog(MultiButtonMsgDlg nvidiaDlg)
        {
            nvidiaDlg.ClickNo();
-            MessageDlg confirmDlg = WaitForOpenForm<MessageDlg>(600000);
+            MessageDlg confirmDlg = AbstractFunctionalTest.WaitForOpenForm<MessageDlg>(600000);


You should use the change I supplied privately:

// We aren't running with admin privilege so we have to say no to the offer to install nvidia libraries var confirmDlg = AbstractFunctionalTest.ShowDialog<MessageDlg>(nvidiaDlg.ClickNo, 600000);

As it is, you're running UI code on the test thread, which causes intermittent failures

bspratt · 2025-01-31T20:19:14Z

pwiz_tools/Skyline/Model/AlphaPeptDeep/AlphapeptdeepLibraryBuilder.cs

-using System;
+/*
+ * Maintainer: David Shteynberg <david.shteynberg .at. proton.me>,
+ *                  MacCoss Lab, Department of Genome Sciences, UW


"Maintainer" isn't something you'll find anywhere in the rest of the code. Just use "Author", which is Skyline-speak for "owner" or "knows more about it than anyone else, presumably". And, use your proteinms email.

bspratt · 2025-01-31T20:23:35Z

pwiz_tools/Skyline/TestUtil/PythonTestUtil.cs

        private void RunNvidiaDialog(MultiButtonMsgDlg nvidiaDlg)
        {
            nvidiaDlg.ClickNo();
-            MessageDlg confirmDlg = WaitForOpenForm<MessageDlg>(600000);
+            MessageDlg confirmDlg = AbstractFunctionalTest.WaitForOpenForm<MessageDlg>(600000);
            Assert.AreEqual(string.Format(ToolsUIResources.PythonInstaller_OkDialog_Successfully_set_up_Python_virtual_environment),
                confirmDlg.Message);
            confirmDlg.OkDialog();


Again, you should use the change I provided privately:
AbstractFunctionalTest.OkDialog(confirmDlg, confirmDlg.OkDialog);
this achieves two things - runs UI code on the UI thread, and makes sure the dialog is completely dismissed before moving along.

bspratt · 2025-01-31T20:40:47Z

pwiz_tools/Skyline/TestUtil/PythonTestUtil.cs

            Console.WriteLine(@"Info: Successfully set LongPathsEnabled registry key to 1");
            _undoRegistry = true;
-            MessageDlg okDlg = WaitForOpenForm<MessageDlg>();
+            MessageDlg okDlg = AbstractFunctionalTest.WaitForOpenForm<MessageDlg>();
            okDlg.OkDialog();


I don't think this is actually reachable code (unless running Skyline as admin, which would be unusual) but this line should be
AbstractFunctionalTest.OkDialog(okDlg, okDlg.OkDialog);

I wonder if there isn't a way we could let a user who does have sufficient rights to set that registry key - launch a tiny app that does only that, and only works if the user clicks through the OS-provided query about it

…rom running AlphaPeptDeep will be logged to the Immediate Window

…yline/work/20240816_pythoninstaller_with_virtual_env_support

…s when "Cancel" is pressed. Update to Python installer to improve user experience.

…rk/20240816_pythoninstaller_with_virtual_env_support

…n SkylineProcessRunner. Refactor PythonInstallerUI a to improve readability. ProcessRunner will not report message to the dialog box when logging to ImmediateWindow to address Brian's concern.

…lision or files between runs. Implement longValidate options for PythonInstaller to enable thorough validation including hash comparisons, the default option lonValidate=false only checks that the expected signatures exist without verifying the hashes which saves time and improves the user experience. Also don't sign the working directory.

…d and configured to verify that this was done properly by the test.

…in AlphapeptdeepBuildLibraryTest that cleans the slate for a more thorough test.

…rary builders only when needed for building (not for verifying.) This to allows testing library build products by hashes, which is now performed as part of AlphapeptdeepBuildLibraryTest.

…yline/work/20240816_pythoninstaller_with_virtual_env_support # Resolved Conflicts: # .gitignore # pwiz_tools/Skyline/SettingsUI/BuildLibraryDlg.resx

PeakTotalIonCurrentArea

a5f1e47

xgwang-uw requested a review from nickshulman August 16, 2024 09:00

nickshulman reviewed Aug 18, 2024

View reviewed changes

pwiz_tools/Skyline/packages.config Outdated Show resolved Hide resolved

xgwang-uw force-pushed the Skyline/work/20240816_pythoninstaller_with_virtual_env_support branch from 868fde7 to 416c9d2 Compare August 20, 2024 19:43

xgwang-uw force-pushed the Skyline/work/20240816_pythoninstaller_with_virtual_env_support branch 3 times, most recently from 3db9cca to b007485 Compare August 21, 2024 22:40

xgwang-uw and others added 10 commits August 26, 2024 12:52

Rename PythonInstaller to PythonInstallerLegacyDlg

c4e7c59

Add PythonInstaller that supports virtual environment

dc612f9

Update to add OneOf dependency with PackageReference

d77d35e

Add RuntimeIdentifier win to Skyline.csproj to fix nuget restore error

9d6cab0

Fix scaling issue and enable locatable for PythonInstallerDlg form

888627c

Fix AutoScaleDimensions value in PythonInstallerDlg.resx

d06c394

Add more verbose logging if "CheckForMissingResources" fails.

3b12574

Fix CodeInspection test by moving PythonInstaller strings from ToolsU…

1e5eb21

…IResources into ToolsResources

Added OneOf.dll to Product-template.wxs

f2d97e9

Fix PythonInstallerTest by consuming strings from ToolsResources

7812cb5

xgwang-uw force-pushed the Skyline/work/20240816_pythoninstaller_with_virtual_env_support branch from a4971e8 to 7812cb5 Compare August 26, 2024 19:58

xgwang-uw added 8 commits August 26, 2024 13:58

Setup python virtual environment and implement building library with …

5c487ab

…alphapeptdeep

Remove unused exception vars

eade5f9

Revert "Setup python virtual environment and implement building libra…

e0a85e1

…ry with alphapeptdeep" This reverts commit 5c487ab. Tempororily remove this commit to make TeamCity build succeeds

Setup python virtual environment and add empty AlphapeptdeepLibraryBu…

15930d7

…ilder class

Add top level logic for BuildLibrary in AlphapeptdeepLibrarybuilder

dab7240

AlphapeptdeepLibraryBuilder - prepare input file for alphapeptdeep fr…

494b457

…om skyline document

AlphapeptdeepLibraryBuilder - prepare settings.yaml file and execute …

622c47a

…alphapeptdeep cli to build library

AlphapeptdeepLibraryBuilder - transform alphapeptdeep output spectral…

668f811

… library file to be skyline compatible

dshteyn and others added 9 commits January 25, 2025 19:19

Refactor and improve code in the process of fixing problem discovered…

1c10711

… during testing. Also changes to the PythonTestUtil related to order of Message Dialogs presented during Python Install and Nvidia configuration.

Address some issue with failing tests.

ec61351

Merge branch 'master' of github.com:ProteoWizard/pwiz into Skyline/wo…

d8200dd

…rk/20240816_pythoninstaller_with_virtual_env_support

Decrement expected number of failures 'Skyline model code must not de…

300e1b6

…pend on UI code'

Implement changes to TestAlphaPeptDeepBuildLibrary and related fixes …

4212ebe

…to PythonInstaller revealed by BPratt's buddy testing

Merge branch 'master' of github.com:ProteoWizard/pwiz into Skyline/wo…

651b777

…rk/20240816_pythoninstaller_with_virtual_env_support

Merge branch 'master' of https://github.com/ProteoWizard/pwiz into Sk…

2d5eeb3

…yline/work/20240816_pythoninstaller_with_virtual_env_support and address issues brought up during buddy testing # Resolved Conflicts: # pwiz_tools/Skyline/SettingsUI/PeptideSettingsUI.cs

Implement suggested changes

c3da42b

bspratt reviewed Jan 31, 2025

View reviewed changes

dshteyn and others added 16 commits January 31, 2025 13:22

Changes in response to bpratt comments on PR3123

fc07478

Implement suggestion from bpratt to improve user experience: output f…

6d928bf

…rom running AlphaPeptDeep will be logged to the Immediate Window

Merge branch 'master' of https://github.com/ProteoWizard/pwiz into Sk…

3da2cf9

…yline/work/20240816_pythoninstaller_with_virtual_env_support

Incorporate bpratt's suggested changes.

6821259

Merge branch 'master' of https://github.com/ProteoWizard/pwiz into Sk…

df735a8

…yline/work/20240816_pythoninstaller_with_virtual_env_support

Clean-up some code cruft. Allow SkylineProcessRunner to Kill a proces…

7893764

…s when "Cancel" is pressed. Update to Python installer to improve user experience.

cruft

a3a17c9

Merge branch 'master' of github.com:ProteoWizard/pwiz into Skyline/wo…

8c259da

…rk/20240816_pythoninstaller_with_virtual_env_support

Make cancellations easier by preventing blocking on reader.ReadLine i…

bde691a

…n SkylineProcessRunner. Refactor PythonInstallerUI a to improve readability. ProcessRunner will not report message to the dialog box when logging to ImmediateWindow to address Brian's concern.

Implement Brian's suggestions to improve user experience.

1ebc4c7

Add check of Python installation to the test after Python is installe…

b68b4ec

…d and configured to verify that this was done properly by the test.

Add a function in PythonInstaller to delete Tools/Python and call it …

43258bf

…in AlphapeptdeepBuildLibraryTest that cleans the slate for a more thorough test.

Switch to standard SafeDeleteLongPath for consistency.

a11b547

Improve the library building organization with timestamps. Create lib…

0646be7

…rary builders only when needed for building (not for verifying.) This to allows testing library build products by hashes, which is now performed as part of AlphapeptdeepBuildLibraryTest.

Merge branch 'master' of https://github.com/ProteoWizard/pwiz into Sk…

d6c2933

…yline/work/20240816_pythoninstaller_with_virtual_env_support # Resolved Conflicts: # .gitignore # pwiz_tools/Skyline/SettingsUI/BuildLibraryDlg.resx

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alphapeptdeep Integration #3123

Alphapeptdeep Integration #3123

xgwang-uw commented Aug 16, 2024

chambm commented Aug 20, 2024

nickshulman commented Aug 20, 2024 •

edited

Loading

xgwang-uw commented Aug 20, 2024

chambm commented Aug 22, 2024

bspratt Jan 31, 2025

bspratt Jan 31, 2025

bspratt Jan 31, 2025

bspratt Jan 31, 2025

bspratt Jan 31, 2025

bspratt Jan 31, 2025

Alphapeptdeep Integration #3123

Are you sure you want to change the base?

Alphapeptdeep Integration #3123

Conversation

xgwang-uw commented Aug 16, 2024

chambm commented Aug 20, 2024

nickshulman commented Aug 20, 2024 • edited Loading

xgwang-uw commented Aug 20, 2024

chambm commented Aug 22, 2024

bspratt Jan 31, 2025

Choose a reason for hiding this comment

bspratt Jan 31, 2025

Choose a reason for hiding this comment

bspratt Jan 31, 2025

Choose a reason for hiding this comment

bspratt Jan 31, 2025

Choose a reason for hiding this comment

bspratt Jan 31, 2025

Choose a reason for hiding this comment

bspratt Jan 31, 2025

Choose a reason for hiding this comment

nickshulman commented Aug 20, 2024 •

edited

Loading