-update Sciex Data API to 1.5.1.391 (May 2024)

[david-cox-sciex]

Update Sciex Data API to 1.5.1.391 (May 2024).

[chambm]

Hi Dave,

Thanks for the update. I think I have previously tried to update to this and found that I couldn't use it because of new non-deterministic file closing. We need a way to close a file or it would be practically impossible to use the API during our memory and handle leak checking. I reported that to Tim, Yunyun and Petr back in May and got

Unfortunately we currently don’t have the capability in Data API for instant closing file. We will add it into our to do list.

as the reply. Did it in fact make it onto the todo list? :)

[david-cox-sciex]

Hi Matt,

Yes, I see the issue with closing and deleting/renaming files. I'm working with the Data API team to see if it can be addressed quickly. I'll try some example dlls in this PR for a bit. Once I have something working, I'll wait for the Data API release, and then use that version.

[david-cox-sciex]

I have the new API working. It adds support for recognizing Echo TOF instruments, and probably more improvements since the 2023 version.
The issue with locking files was solved by calling CloseFile on the API.
I need to wait for an official release of the API, then I will update this PR with it.
If you see changes I can make while I'm waiting for that updated dll, let me know.

[brendanx67]

The release consideration makes me wonder if we have a license for this version?

[brendanx67]

Was meaning to cancel my consideration, but now that it is part of the thread, Tim Blacker has raised the need to update our license in the past. Do you have an estimate for when an "official release" might happen?

[david-cox-sciex]

I was hoping to use the current release, but the issue with file locking stopped me. I'd rather wait for the next release so it handles these locked files in a better way.
I'll confirm with Tim Blacker before putting the final API dll in for this review. I think the release of this next API is going to be very soon, but releases can get held up for reasons that happen in meetings I don't get invited to (thankfully).

[chambm]

Can the wiff->close() call just be handled by the destructor (and WiffFile2's destructor)?

[david-cox-sciex]

I think so. I'll remove the close and handle it by destructor.

[chambm]

The *List_ABIs have shared_ptr to a WiffFileImpl (or WiffFile2Impl). It's WiffFile2Impl's destructor that should call close, because it'll be called when both *Lists are destroyed. Calling close manually in the *List destructors is unsafe as the other List may still be using the WiffFile.

[david-cox-sciex]

WiffFile2Impl destructor was calling close, but then this part of VendorReaderTestHarness was preventing the file from being renamed later. I'm not sure how to handle it.

        if (msd_reverse.run.spectrumListPtr.get())
            for (size_t j = 0, end = msd_reverse.run.spectrumListPtr->size(); j < end; ++j)
                msd_reverse.run.spectrumListPtr->spectrum(end - j - 1);

[chambm]

Are you saying that there's a WiffFile2Impl that hasn't been destroyed by the time the rename part of the test happens? Or there's some other way it's preventing the rename? Because I'm not sure how the WiffFile2Impl would survive after msd_reverse goes out of scope (that should destroy both its spectrumListPtr and chromatogramListPtr and thus destroy the shared_ptr they hold).

[david-cox-sciex]

I can't figure out why Wiff2Impl isn't being destroyed in that part of the test. There is no issue on the similar line:
bfs::remove_all(newRawPath); // remove the copy of the RAW file with non-ASCII characters

but without close in the SpectrumList destructor, the line:
bfs::rename(rawpath, rawpath + ".renamed");

fails.
There should be no issue with calling close multiple times. It is only closing a SQL connection. I think keeping the close in SpectrumList and ChromatogramList is okay. Another option would be to make a special case in VendorReaderTestHarness similar to:
if (bfs::exists(bfs::path(rawpath) / "Analysis.yep")

Let me know which option you prefer or if you can think of something else.

[chambm]

I'll take a look at this.

[david-cox-sciex]

Adding a config section for the logging will stop DataAPI from excessive logging. However, it requires including log4net.dll. I'm not sure how to include this for the Reader_ABI_Test.exe program.

[chambm]

In vendor_api/ABI/Jamfile.jam's vendor-api-requirements:
result += <assembly-dependency>$(PWIZ_ROOT_PATH)/pwiz_tools/Shared/Lib/log4net.dll ;

in install-requirements:
result += <source>$(PWIZ_ROOT_PATH)/pwiz_tools/Shared/Lib/log4net.dll ;

See if that works.

[david-cox-sciex]

the version of log4net that Sciex is using appears to be older than the one Skyline is using. When I try to use the newer version, the configuration doesn't apply and it reverts back to the very large logging.

I can try using two different versions of log4net:
https://stackoverflow.com/questions/3158928/referencing-2-different-versions-of-log4net-in-the-same-solution

Or is there a better option?

[david-cox-sciex]

I've found a way to embed the OFX.Logging.dll and log4net.dll in the Data API assembly; this enables configuration of logging. Works on the Reader test, I'll run TC to confirm other tests are passing.

[david-cox-sciex]

Well, that seems to break some Skyline tests that use logging. I've asked the Sciex API group to look into it, but it will likely be a fix for the next version.
The current release of MSConvert with the very old Data API also has the large log files. The December 2024 Data API also has the large log files, but still fixes the Echo MS and file locking issues.
Can we proceed with the Dec 2024 Data API?