MDSVALUE in MATLAB breaks when reading Signals about mdsplus HOT 20 OPEN

Hi @ModestMC -- I have not yet reviewed the "Array of Pointers to Descriptors" (APD) changes in detail. They originated with PR #2620 (and also require pending PR #2661). My assumption (perhaps incorrect) is that the APD feature applies to all APIs.

If GA uses APDs in its queries, then it would be best to take the entire PR #2661.

However, if GA never uses APDs, then it might be possible to use "alpha 7-139-49" as the base for the GA branch. Note though that "7-139-49" predates the IDL fixes made in October, thus there would be several cherry picks that GA would probably want.

I am presently working on porting the IDL tests to MATLAB. Initially, the MATLAB test suite will be run manually. But after the maintenance work on the build server is completed, the MATLAB tests will run automatically as part of the PR review process.

from mdsplus.

The problem is due to a recent improvement of the Connection object that now is also able to transfer data types other than scalars and array. However the signal that now shows up as result of Connection.get() was not properly handled by the matlab routine JavaToMatlab.m I will fix this in a couple of days.

from mdsplus.

I attempted a revert of the improvement described in PR #2620 and triggered a rebuild and the error still occurs. What from the changes you introduced at this time would have broken the feature?

I tried loading an older version that predates any of these feature sets (alpha_release-7-108-1) and my autobuilder broke, but in reading over the code I'm not seeing too much.

from mdsplus.

Looking into the code, I noted that in the Java Connection there is a (wrong) possibility that in some condition the expression gets directly evaluated (without data()). This would explain what you see and is not related to the APD stuff. I will issue later a PR so that you cat try it

from mdsplus.

You may try branch gm-fix-apd that should contain, among others, a fix to the problem (basically due to the fact that the java Connection.get() method did not force a data() evaluation).
The checks for the PR are in progress now, but meanwhile you can check that branch in order to verify the problem.

from mdsplus.

The Jenkins server is undergoing maintenance. In the meanwhile, we will run the tests manually. Can you GA verify that Gabriele's branch fixes this problem?

from mdsplus.

I pulled the new branch and attempted to use it, but got the same error. What did you guys use to test the fix on your end?

from mdsplus.

Hmmm.. really strange, we tested it on our side on a whole set of MATLAB tests (not yet on jenkins). Could you send me the failing MATLAB code and possibly a description (e.g. via TCL show data) of the accessed node?

from mdsplus.

BTW the fix is not in the MATLAB MDSplus library, but in the Java mdsobjects library. Did you update it?

from mdsplus.

I believe these are the changes to the Java mdsobjects library you're describing.

Here is the data inside of the node itself that breaks when mdsvalue is called.

It also fails when the node is not self-referential.

from mdsplus.

Are you sure that your MATLAB is using the right jar? Changing jars version on MATLAB is sometimes tricky....
We tested our version (branch gm-fix-apd) exactly in the same condition of your example and everything works as expected (while the current alpha HAS the problem)

from mdsplus.

Did you get a chance for checking this?

from mdsplus.

@GabrieleManduchi Not exactly. I've been having to debug our clusterwide MATLAB installation's way of loading MDSplus. For the past several years, we have made MDSplus available to MATLAB by having one symlink that's used for the java modifications MDSplus needs (two lines in classpath.txt and one line in the librarypath.txt). It turns out there's no good way to point the symlink at a user development version of MDSplus (also it means our MATLAB version is functionally pinned).

Anyways I've now spent probably half a day's worth of time collectively trying to figure out if I can somehow get lmod to be able to load in a particular version of MDSplus (we have a way of doing this for the development versions as well) and have MATLAB pull the necessary paths from the environment variables. I'm not sure there's a way to do this (see here), so for now I'm willing to accept defeat.

If you're sure about your fix, I think our only option is going to be to take it on faith that it works and release a new build for our cluster. I got official blessing to monkey with the clusterwide installation to test out the fix and it correctly retrieved signals. I couldn't do any serious/robust testing, but I think this will eliminate the main blocker.

from mdsplus.

The gm-fix-apd branch passed manual testing on Ubuntu 20 for this fix. See posts in PR #2661.

from mdsplus.

Gabriele's posts of 11-Dec-2023 (above) indicate that only some of the PR #2661 files are needed to fix this MATLAB bug. Thus, I created an experimental (private) branch with just the following files from the PR.

deploy/packaging/debian/kernel.noarch
deploy/packaging/redhat/kernel.noarch
include/mdsobjects.h
java/mdsobjects/src/main/java/MDSplus/Connection.java
javamds/mdsobjects.c
mdsobjects/cpp/mdsipobjects.cpp
tdi/treeshr/TreePutDeserialized.fun

When using the experimental branch, MATLAB was able to retrieve and plot a signal. (And for the sake of completeness, I also confirmed that the current alpha branch does fail with the errors shown in the initial bug report.)

Note though that the test-tab.ans file still needs to be updated with the new TreePutDeserialized.fun function. However, the experimental branch passed all other automated tests, and also passed the IDL tests.

If the decision is made to split PR #2661 into new PRs -- one for MATLAB, and the other for APD (aka "Array of Pointers to Descriptors") -- it would be best to have Gabriele review the proposed split.

from mdsplus.

I thought they were complaining that MATLAB did. not deal with APDs.

from mdsplus.

Please keep the whole branch since it contains a set of(related) improvements for handling apds over connection, a feature requested by tcv and already extensively tested.

from mdsplus.

Hi @joshStillerman and @GabrieleManduchi -- Thanks for both of your posts. I was merely doing research so that PSFC can discuss the options when the software team meets in early January.

Josh -- General Atomics was not reporting an issue with "array of pointers to descriptors" (APD). This bug #2660 is a more fundamental issue, namely that the MATLAB API cannot retrieve data from a signal. I reproduced that bug by attempting to retrieve a signal from a C-Mod shot. The retrieval failed with the same errors shown in the initial bug report. Note that Gabriele posted above that the fix for the MATLAB signal issue has been included in PR #2661 for the APD issue.

Gabriele -- Thanks for the advice. I do not intend to split PR #2661 into two PRs. Note though that GA has requested its own branch. I was merely researching options in case GA requests that their branch have a MATLAB-only fix (i.e., if GA wants to exclude the APD fix).

Note -- The long-term goal is to run MATLAB tests as part of the build system (however that requires adding some infrastructure). A good starting point will be to write MATLAB test cases that are comparable to those in the IDL Test Harness.

from mdsplus.

Results of additional experiments . . .

alpha_release-7-139-49 (PR #2622, 22-Sep-2023, 21ead65) = MATLAB is able to retrieve and plot a signal

alpha_release-7-139-50 (PR #2620, 23-Sep-2023, d996b0c) = MATLAB throws errors when attempt to retrieve a signal

from mdsplus.

@mwinkel-dev can you tell me what all these APD fixes will impact in the codebase (eg. what all we'd want to test internally before using them)? I assume you've already looked over the same files I was about to go read for myself, figure this is simpler.

from mdsplus.