CDR Tickets

BZDATETIME::2011-03-22 11:15:26
BZCOMMENTOR::Alan Meyer
BZCOMMENT::1

I've been wrapped up in the Citation Management System, but
I'll suspend that and work on this for now.

BZDATETIME::2011-03-22 21:59:20
BZCOMMENTOR::Alan Meyer
BZCOMMENT::2

This is a scary one. It looks to me like the problem must have
existed since 2004.

The program was originally written in 2002 for the purpose of
adding or deleting various kinds of links in InScopeProtocols.
This was one of our first global changes, written well before the
ModifyDocs module was written. It was a pretty big program that
handled six different kinds of global changes, using eleven
different filters, some of which, like the ones that modified
Terminology links, accepted parameters to handle various
different elements.

Later, in 2004, we decided to modify the program to handle
CTGovProtocols as well as InScopeProtocols (see OCECDR-1209). The
modifications involved changes to a lot of things including the
programs, XSLT filters, the CTGovProtocol schema, the
CTGovProtocol import program, and a global change to modify
CTGovProtocols to match the new schema.

It appears that the XSLT filter changes necessary to add
Condition Terminology links to CTGovProtocols never got made. I
see the changes in the Delete filter, but not the Add filter.

I don't know how this bug got through testing and has been in
place for all of these years without being noticed. I'm guessing
(or maybe just hoping) that the majority of uses did not involve
adding Conditions.

At this point I think I have to examine both the Add and Delete
filters to see if any other of the required changes for other
kinds of terminology were not made. If not, I'll make them.
I'll also try to confirm that the relevant parts of the
CTGovProtocol schema still match the view of those document types
that is embedded in the filters.

I'm seeing the following Terminology elements that can be
changed:

Diagnosis
ExclusionCriteria
Condition
Gene
InterventionType
InterventionNameLink

I'll need to check the schema for CTGovProtocol to find out which
of these are included, and check the filters to make sure that
filter rules exist and are up-to-date for both Add and Delete for
each of them, and fix any that need fixing.

Testing will be a lot of work. Maybe the reason we're in trouble
on this is that is was more work than we realized we had to do
back when we needed to do it. Ideally, we ought to run global
change tests on each of the two document types, for each of the
six Terminology types, for each of the two transaction types (add
and delete), for a total of 24 tests. However we should be able
to combine a number of these in a few global changes in order
to reduce the number of separate test runs we have to make,
though not the number of separate checks we have to make on the
outcomes. If we are sure we aren't going to do any more global
changes on InScopeProtocols we might be able to cut corners on
testing with those documents.

Since this program was written before we had the ModifyDocs
module, it doesn't have the live/test mode capability that all of
the later globals have. So we'll have to test in live mode on
Mahler and/or Franck.

Since this is an important task, I'm adding Bob and Volker to the
CC list.

While I work on this I'm also working on the CiteMS. Both tasks
are currently marked with priority P5. I'll divide my time more
or less equally between the two. If this one is more important,
please raise the priority to P4 or whatever seems appropriate.

BZDATETIME::2011-03-22 23:33:26
BZCOMMENTOR::Alan Meyer
BZCOMMENT::3

There is some good news.

I've read through the Terminology Add filter and it looks to me like Condition was the only Terminology type that was broken.

If I'm right, this should be an easy fix. But I'd still like us to do some extensive testing just in case I'm not right.

I'll do some more work on it on Thursday.

BZDATETIME::2011-03-24 20:37:49
BZCOMMENTOR::Alan Meyer
BZCOMMENT::4

It turns out I was wrong on at least two counts.

The global does indeed, as William thought, support test mode output. It doesn't use the ModifyDocs module but does use the cdrglblchg modules, which ModifyDocs also uses, to prepare the test mode outputs.

Secondly, Condition is not the only terminology type affected. Gene also didn't work.

I believe that I've now got both of those working, but I'd like to do more testing before I turn it over for user testing. Mahler is going down soon to apply Windows patches so I plan to resume this next Tuesday.

BZDATETIME::2011-03-29 23:45:06
BZCOMMENTOR::Alan Meyer
BZCOMMENT::5

I found more problems. They were all due to the fact that schema
changes had been introduced that caused the XSLT filter to skip
elements, but no one noticed that and the filter was not updated.

I have fixed everything I could find except the ArmOrGroupLink
and InterventionDescription elements. I couldn't find any
documents on Mahler or Bach with those elements. I'd like some
if we have any in order to test the filter on them before I put
changes into the CDR. The documents don't have to be in the CDR
now. They can be plain files. If someone has some
CTGovProtocols in his pocket with the new ArmOrGroupLink and
InterventionDescription elements, please forward them to me, or
forward CDR IDs.

Otherwise, this is ready for testing on Mahler. If everything
works correctly, there should be no "Errs" results for documents
that were valid before the global change. There should also be
no elements in the "Old" versions of documents that got
incorrectly dropped in the "New". That could happen if a schema
change occurred and I didn't properly update the filter to
account for it.

Please test in test mode on Mahler with whatever complex
terminology changes you can think of, especially with changes
that will affect large numbers of documents so that we're more
likely to see a problem if one is lurking in the XSLT script.

I won't mark this resolved-fixed until we decide what to do about
the two new elements (ArmOrGroupLink and InterventionDecision).

BZDATETIME::2011-03-30 10:35:56
BZCOMMENTOR::Bob Kline
BZCOMMENT::6

Unlike one-off global change jobs, for which we don't need to worry about future changes to a document type's schema, this tool might benefit from a check that goes something like this:

1. create a set of expected top-level element names
2. get the list of top-level elements from the schema
3. if any element in the list from step 2 is not in the step 1 set, abort

The only part which isn't trivial is step 2. I've added a method to the class returned by cdr.getDoctype() which takes care of that part:

#---------------------------------------------------------------------
import cdr
docType = cdr.getDoctype('guest', 'InScopeProtocol')
expected = set(docType.getChildren('InScopeProtocol'))
#---------------------------------------------------------------------

The return value from the method is a sequence of element names, in the order that the schema allows them to appear in the block, so if you needed to you could ensure that the order matches what you expect (and it's possible that the same name might show up more than once, as was the case with top-level Comments in Term documents until very recently). I've just collapsed them into a set here, but you'll know better which approach is needed.

If you invoke docType.getChildren() with no arguments, it will use the document type name as the parent name, retrieving the top-level elements for the document type, but you can get children for any complex element in the document type. (If you invoke it for a non-complex element you'll get back ['EMPTY'] or ['PCDATA'], whichever is appropriate.)

If we do this, we can make the global change job will fail when we change the schema in a way that might break the filter, instead of silently mangling the data.

The new method is installed on Mahler and checked into svn.

What do you think?

BZDATETIME::2011-03-30 12:04:08
BZCOMMENTOR::Alan Meyer
BZCOMMENT::7

(In reply to comment #6)
> Unlike one-off global change jobs, for which we don't need to worry about
> future changes to a document type's schema, this tool might benefit from a
> check that goes something like this ...

I think it's a good idea. I think it might have prevented some of the errors that accumulated in the script. There were also errors that I think it wouldn't have caught.

I'll go over it with you on Thursday. I may be doing something in the XSLT that could be done a better way that isn't as sensitive to schema changes.

Also, I forgot to note that we have a pysvn problem. When I installed the new filter on Mahler the filter manager successfully put the filter into the database but failed to update the subversion repository. I suspected a permissions problem, and it may very well be, but it might be more complex than that.

I followed what happened in a log file that Volker creates. It successfully created a temporary directory. The directory exists. But it apparently stalled and timed out trying to checkout the filters into it. I don't think it's a problem with my userid or password. Maybe the three of us can look at that on Thursday also.

BZDATETIME::2011-03-31 17:16:49
BZCOMMENTOR::William Osei-Poku
BZCOMMENT::8

(In reply to comment #5)

> I have fixed everything I could find except the ArmOrGroupLink
> and InterventionDescription elements. I couldn't find any
> documents on Mahler or Bach with those elements. I'd like some
> if we have any in order to test the filter on them before I put
> changes into the CDR. The documents don't have to be in the CDR
> now. They can be plain files. If someone has some
> CTGovProtocols in his pocket with the new ArmOrGroupLink and
> InterventionDescription elements, please forward them to me, or
> forward CDR IDs.
>
>

Here are two trials that have the two elements you mentioned above:

CDR598391
CDR649750

They are InScopeProtocol documents so they are not CTGovProtocol elements as I thought.

BZDATETIME::2011-03-31 22:58:28
BZCOMMENTOR::Alan Meyer
BZCOMMENT::9

There was indeed an error in the selection criteria. The handling of CTGovProtocol exclusion criteria was incorrect (sigh). I think I fixed it. It's another thing that needs to be tested.

Figuring out what damage may have been done and fixing it is going to be a pretty big task. I'll have to think about that.

BZDATETIME::2011-03-31 23:52:28
BZCOMMENTOR::Alan Meyer
BZCOMMENT::10

(In reply to comment #8)
...
> Here are two trials that have the two elements you mentioned above:
...

Thanks.

Sometimes I suffer from hysterical blindness and fail to see obvious things.

I've updated the filter and tested with the docs. I think I'm preserving the values correctly.

BZDATETIME::2011-04-07 20:29:23
BZCOMMENTOR::Alan Meyer
BZCOMMENT::11

This is ready for user testing. Tests should be done on Mahler, in test mode. More suggestions concerning testing can be found in comment #2.

In addition to testing, there is one more thing I would like to do under this task - to integrate Bob's schema check routine into the global change. I'll call Bob's routine to see if all of the elements I expect are in the document, in the places and orders I expect them. If they aren't I'll make the global produce a message and quit before the user fills out the form or the system modifies any documents.

I'll create a separate task to look back over old documents, fishing for possible errors that may have occurred due to schema / xslt filter divergence. Depending on what I find, some cleanup may be required.

BZDATETIME::2011-04-11 14:00:48
BZCOMMENTOR::William Osei-Poku
BZCOMMENT::12

I have started testing (in test mode on Mahler) and have come across the following issues. I will let you take a look at them before I continue testing. Here are the selection criteria:

Changing doc types: InScopeProtocol CTGovProtocol
Protocol status: Active
Any of Condition: lip and oral cavity cancer;Index term;Protocol selection criteria;Cancer diagnosis (40091)
Any of Condition: oropharyngeal cancer;Index term;Protocol selection criteria;Cancer diagnosis (38965)
Not Condition: tongue cancer;Index term;Cancer diagnosis (654585)
Add Condition: tongue cancer;Index term;Cancer diagnosis (654585)

1. The status email I received has many messages unrelated to the current global.
2. The test results pages for this global also appear to have a lot of other changes going on. For example: deleting and adding organizations.

BZDATETIME::2011-04-11 15:20:46
BZCOMMENTOR::Alan Meyer
BZCOMMENT::13

(In reply to comment #12)
> I have started testing (in test mode on Mahler) and have come across the
> following issues. ...

Thanks William.

I'll have a look at it tomorrow.

BZDATETIME::2011-04-12 21:39:33
BZCOMMENTOR::Alan Meyer
BZCOMMENT::14

(In reply to comment #13)
...
> I'll have a look at it tomorrow.

It took me a while to figure this out. It's scary looking and
maybe should be changed, but it's not actually a problem.

Here's what's happening:

The "old" version is being extracted from the database and
displayed with full re-denormalization.

The "new" version is being extracted from the database with
no new denormalization.

So what's happening is that we're seeing links that are identical
but, paradoxically, the "old" version shows the latest version of
the denormalized content while the "new" version shows the
version of the denormalized content at the time that the version
was last saved before the global change.

The real content of the links is identical. It's only the
"fast denormalization" that differs.

This problem has been there all along but we probably didn't
notice it when I developed the code because the system wasn't as
old as it is now and denormalization strings may not have changed
much. Or maybe we noticed it but it happened infrequently enough
that we didn't think it needed to be changed - since no actual
harm occurs.

It works this way because:

The display of the old record is done by extracting the
record from the database using the fast denormalization
filter. We don't seem to have a Python interface that gets
records any other way - though we could write one.

But the filtering is done by running the database record
through the global change filter without passing it through
the denormalization filter - which seemed like the safest
approach at the time because it avoided fetching the document
from the server and then sending it back again for
transformation.

We can change this if desired. If we do, we should create a new
Bugzilla task for it.

In the meantime, we should be aware that denormalization strings
can be different between the old and new versions. So when you
see things like:

• M. D. Anderson Cancer Center at University of Texas
  + M.D. Anderson Cancer Center

it's not actually a problem in the document.

BZDATETIME::2011-05-04 10:19:21
BZCOMMENTOR::William Osei-Poku
BZCOMMENT::15

Trying to capture all the offline exchanges about this issue.

---Original Message---
From: Osei-Poku, William
Sent: Wednesday, May 04, 2011 9:24 AM
To: 'Alan Meyer'
Cc: Volker Englisch; Bob Kline
Subject: RE: FW: [OCECDR-3324] Global Change Protocol Links / Modify terminology in protocols

>But in the meantime, it appears to be working normally again with
>nothing other than a stop and restart.

> Alan

Can I continue testing or I should wait? If I can continue testing, should I also test in live mode also? (You had given an earlier instruction not to test in live mode).

Thanks,
William

---Original Message---
From: Alan Meyer vrmeyer@comcast.net
Sent: Tuesday, May 03, 2011 10:53 PM
To: Volker Englisch
Cc: Osei-Poku, William; Kline, Robert (NCI)
Subject: Re: FW: [OCECDR-3324] Global Change Protocol Links / Modify terminology in protocols

Looks okay to me:

rw-rw-rw 1 user group 45562850 May 3 22:42 publish.log

Alan

On 5/3/2011 10:36 PM, Volker Englisch wrote:
> Could you please check if the permissions for the publish.log file are OK.
>
> It may be that I had tested my changes to the publishing service but
> forgot to reset the file permission.
>
> I'll have a look tomorrow.
>
> Volker
>
> On 05/03/2011 06:30 PM, Alan Meyer wrote:
>> There may be a new bug in the publishing service software, which is also
>> responsible for initiating batch jobs for reports, global changes, or
>> any other jobs that run in the background.
>>
>> There were three batch jobs in the queue, two reports and one global
>> change. All were waiting to get started.
>>
>> The CdrPublishing service was supposed to notice the three jobs and
>> start them. The service appeared to be running. Windows said it was
>> running, both in the services query and in the task manager. But it
>> didn't appear to be actually doing anything. There were no recent "Top
>> of processing loop" log messages indicating that the service was
>> actually running, and there haven't been any since April 21, when some
>> changes had been made to the program.
>>
>> I halted the publishing service and ran it by hand in the debugger. It
>> started all three jobs when running for me. I broke out of it (which
>> also killed the global change test job that it had spawned), reset the
>> status flag for the global change test program to "Queued", and started
>> the publishing service.
>>
>> This time it picked up the global change job and started it normally.
>> It also began writing the standard log messages that establish that it
>> is alive.
>>
>> The global change ran to completion.
>>
>> If Volker has a chance, maybe he can look at it. If he's saturated with
>> other stuff, I'll work on it.
>>
>> But in the meantime, it appears to be working normally again with
>> nothing other than a stop and restart.
>>
>> Alan
>>
>> On 5/3/2011 1:26 PM, Osei-Poku, William wrote:
>>> OK. Sounds good. Thanks!
>>>
>>>
>>> ---Original Message---
>>> From: Alan Meyer vrmeyer@comcast.net
>>> Sent: Tuesday, May 03, 2011 1:25 PM
>>> To: Osei-Poku, William
>>> Subject: Re: FW: [OCECDR-3324] Global Change Protocol Links / Modify
>>> terminology in protocols
>>>
>>> It appears that the job never actually started. I don't know why. The
>>> batch job starter may be, as you say, stuck. Or it may have encountered
>>> an error while trying to start.
>>>
>>> I'll figure it out however I've got a meeting at 1:30 and another at 3,
>>> so I may not get to it until late this afternoon. I'll let you know
>>> before I leave tonight whether I've fixed it or not.
>>>
>>> Alan
>>>
>>> On 5/3/2011 1:06 PM, Osei-Poku, William wrote:
>>>> Hi Alan,
>>>> I am testing this in Mahler. But the first global I ran appears to
>>>> have stuck. It's been over an hour since I ran it. Could you please
>>>> take a look?
>>>> Thanks,
>>>> William
>>>>

BZDATETIME::2011-05-05 10:34:00
BZCOMMENTOR::William Osei-Poku
BZCOMMENT::16

Verified on Mahler. Please promote changes to Bach.

BZDATETIME::2011-05-05 10:42:32
BZCOMMENTOR::Alan Meyer
BZCOMMENT::17

I've promoted the modified program to Bach.

As it stands, the program will refuse to work with Week_016, Week_017, and possibly other files because the spreadsheets in those files lack the expected row of column headers in the top row.

Personally, I like the idea of keeping the headers intact. If anyone edits anything, for example in the notes sections, it may help to avoid errors. I also wonder why some spreadsheets were created with them and some without since I thought they were all created by the same process.

However, if this is a pain I can modify the program to work with or without headers.

I will not change anything unless and until someone says that I should.

BZDATETIME::2011-05-06 00:14:05
BZCOMMENTOR::Alan Meyer
BZCOMMENT::18

(In reply to comment #17)
> I've promoted the modified program to Bach.

I mistakenly entered a comment above that belonged under OCECDR-3327. Please ignore it.

I have installed the modified filter for this task in version control and in the database on Bach.

Please make the first run that uses it on Bach in test mode to be sure that I didn't make any mistakes in promoting the software.

BZDATETIME::2011-05-12 13:56:35
BZCOMMENTOR::William Osei-Poku
BZCOMMENT::19

(In reply to comment #18)
> (In reply to comment #17)
>
> Please make the first run that uses it on Bach in test mode to be sure that I
> didn't make any mistakes in promoting the software.

The changes have been tested on Bach. We also did a live run on Bach and it came out well.

BZDATETIME::2011-07-01 15:24:21
BZCOMMENTOR::Bob Kline
BZCOMMENT::20

[reposted for Alan]
I implemented a schema check inside the Global Change to detect
any future mismatch between the Add Terminology filter and the
schemas.

There were different ways to do it. The finest grained approach
would check the schema only when it absolutely needed to. If the
CTGovProtocol schema was okay but the InScopeProtocol had
changed, I could balk only on InScopeProtocols and only when
adding a term to one of them. Everything else would continue to
work.

A problem with that approach was that the errors only appeared
in the email report, after the job ran and changed some documents
but not others.

So I finally decided to check both CTGov and InScope schemas
before ANY terminology change. Then I could halt the program
right in front of the user, without ever starting the batch job.

If the schema definitions change for either of:

/InScopeProtocol//ProtocolDesign
or
/CTGovProtocol//PDQIndexing

the program will halt, write a message to a log file, and display
the message on screen without starting the global change batch
job. The user will be asked to call a programmer to fix the
problem.

I've tested on Mahler. I have not promoted it to Bach.

The program is not yet under version control because of our Linux
system problems.

I don't think there's really anything to test. I ran the program
with a correct view of the schemas and with incorrect views. It
worked with the correct view and produced the error with the
incorrect view. I think it's safe to put on Bach because the
worst that the new change can do is make the program refuse to
run. It doesn't alter the actual global changes in any way. But
I'll hold off promoting it until someone says it's okay.

BZDATETIME::2011-07-06 00:35:36
BZCOMMENTOR::Alan Meyer
BZCOMMENT::21

I've gone over everything fairly carefully, including:

The global change Python code.

All nine XSLT filters invoked by that code.

The CTGovProtocol and InScopeProtocol schemas.

I checked that:

The code, filters, and schemas all agreed with each other.

Only one of the filters was actually at risk due to schema
changes.

The code now confirms that, for that filter, all of the
schema elements that must match the filter do match it.

I also ran some more tests to be sure everything was right.

The only changes I made to the program were:

Added checks to halt processing if the schemas have changed
in the critical areas used by the Add Terminology filter.

Added comments so that I or another programmer will
understand what to do if processing halts with an error
message about a schema / filter mismatch.

There were no changes to the global change code itself. All the
changes involved checks that are made before the global change
actually starts.

As I said in the last comment, I don't think there's really
anything to test beyond the testing I've already done and William
has already done on the earlier filter changes. However I will
wait to promote this to Bach until someone tells me to proceed.

In the meantime, I've checked the code into version control and
am marking this resolved fixed.

BZDATETIME::2011-07-14 15:21:56
BZCOMMENTOR::William Osei-Poku
BZCOMMENT::22

I think it is OK to promote this to Bach.

BZDATETIME::2011-07-19 11:21:29
BZCOMMENTOR::Alan Meyer
BZCOMMENT::23

The modified program is now on Bach and Franck.

BZDATETIME::2011-08-03 20:57:40
BZCOMMENTOR::William Osei-Poku
BZCOMMENT::24

(In reply to comment #23)
> The modified program is now on Bach and Franck.

Closing issue. Thank you!