Support for Aggregate CAS Multiplier #25

mac-op · 2024-08-20T01:57:18Z

What's in the PR

This PR introduces support for Aggregate CAS Multipliers and the supporting facilities. Additions include:

FlowController and its child class FixedFlowController [1]. Custom Flow Controllers and Flow Controller descriptor files are not yet added.
Step class and Flow interface [2].
New CAS::release method to allow the CAS to release itself, similar to the Java API [3].
New error message type and ID in case of invalid CAS release.
New exception: EngineProcessingException.
Changes in internal API:
- New constructor for CASPool to allow it to keep track of its AnnotatorContext owner.

Tests
The first test case added in #22 is now passing. The second test case should still be revised, as outlined in #24
Edit: #24 is now covered after the latest commit.

TODO: Documentation and exception handling

No longer have to cast FlowController to FixedFlowController when we want to find an engine

Also introduced new Exception: EngineProcessingException

FlowController is not deleted on destruction

The constructor for the UNSPECIFIED case didn't initialize the union properly, so when SimpleStep's assignment operator is called later on it can segfault when assigning string.

The union needed to be properly destroyed in assignment operator, and initialized in copy constructor and AO

This introduces a new error ID and error message UIMA_MSG_ID_EXC_INVALID_CAS_RELEASE

DrDub

annotator_mgr diff is very big and full of whitespace changes. Please remove all the whitespace changes from this diff in all files, especially in annotator_mgr so I can finish the review.

DrDub · 2024-08-20T08:50:04Z

src/test/src/test_engine.cpp

@@ -635,9 +636,9 @@ void mainTest(uima::util::ConsoleUI & rclConsole,
    testCallingSequence2(rclConsole, cpszConfigFilename);
    testCallingSequence3(rclConsole, cpszConfigFilename);
  }
-  testCasMultiplier(rclConsole);
+  testCasMultiplier(rclConsole);  testAggregateCASMultiplier(rclConsole);


Why ran it three times?

src/test/src/test_engine.cpp

src/test/data/descriptors/SimpleTextSegmenter.xml

src/framework/uima/msgstrtab.h

DrDub · 2024-08-20T09:01:35Z

src/framework/uima/internal_engine_base.hpp

@@ -64,6 +64,10 @@ namespace uima {
 /*       Types / Classes                                                   */
 /* ----------------------------------------------------------------------- */
 namespace uima {
+


So there's no other exception already defined by the framework that we could use? The framework already declares other 80 exceptions:

uima-uimacpp/src$ find . -name \*.hpp -exec grep -H UIMA_EXC_CLASSDECLARE \{} \; ./cas/uima/featurestructure.hpp: UIMA_EXC_CLASSDECLARE(InvalidFSObjectException, CASException); ./cas/uima/featurestructure.hpp: UIMA_EXC_CLASSDECLARE(FeatureNotAppropriateException, CASException); ./cas/uima/featurestructure.hpp: UIMA_EXC_CLASSDECLARE(IncompatibleValueTypeException, CASException); ./cas/uima/featurestructure.hpp: UIMA_EXC_CLASSDECLARE(FSIsNotStringException, CASException); ./cas/uima/featurestructure.hpp: UIMA_EXC_CLASSDECLARE(WrongStringValueException, CASException); ./cas/uima/arrayfs.hpp: UIMA_EXC_CLASSDECLARE(FSIsNotArrayException, CASException); ./cas/uima/arrayfs.hpp: UIMA_EXC_CLASSDECLARE(FSArrayOutOfBoundsException, CASException); ./cas/uima/fsindexrepository.hpp: UIMA_EXC_CLASSDECLARE(InvalidIndexIDException, CASException); ./cas/uima/typesystem.hpp: UIMA_EXC_CLASSDECLARE(TypeSystemAlreadyCommittedException, CASException); ./cas/uima/typesystem.hpp: UIMA_EXC_CLASSDECLARE(InvalidFSFeatureObjectException, CASException); ./cas/uima/typesystem.hpp: UIMA_EXC_CLASSDECLARE(InvalidFSTypeObjectException, CASException); ./cas/uima/casexception.hpp: UIMA_EXC_CLASSDECLARE(CASException, Exception); ./cas/uima/casexception.hpp: UIMA_EXC_CLASSDECLARE(NotYetImplementedException, CASException); ./cas/uima/casexception.hpp: UIMA_EXC_CLASSDECLARE(SofaDataStreamException, CASException); ./cas/uima/cas.hpp: UIMA_EXC_CLASSDECLARE(CouldNotCreateFSOfFinalTypeException, CASException); ./cas/uima/cas.hpp: UIMA_EXC_CLASSDECLARE(DuplicateSofaNameException, CASException); ./cas/uima/cas.hpp: UIMA_EXC_CLASSDECLARE(InvalidBaseCasMethod, CASException); ./cas/uima/xmltypesystemreader.hpp: UIMA_EXC_CLASSDECLARE(XMLTypeSystemReaderException, uima::Exception); ./cas/uima/lowlevel_typesystem.hpp: UIMA_EXC_CLASSDECLARE(FeatureIntroductionFailedException, CASException); ./cas/uima/lowlevel_typesystem.hpp: UIMA_EXC_CLASSDECLARE(TypeCreationFailedException, CASException); ./cas/uima/fsindex.hpp: UIMA_EXC_CLASSDECLARE(InvalidIndexObjectException, CASException); ./cas/uima/fsindex.hpp: UIMA_EXC_CLASSDECLARE(WrongFSTypeForIndexException, CASException); ./cas/uima/listfs.hpp: UIMA_EXC_CLASSDECLARE(FSIsNotListException, CASException); ./cas/uima/listfs.hpp: UIMA_EXC_CLASSDECLARE(ListIsEmptyException, CASException); ./cas/uima/listfs.hpp: UIMA_EXC_CLASSDECLARE(ListIsCircularException, CASException); ./cas/uima/internal_casdeserializer.hpp: UIMA_EXC_CLASSDECLARE(DeserializationException, Exception); ./framework/uima/exceptions.hpp: UIMA_EXCEPTION_DESCRIPTION, UIMA_EXC_CLASSDECLARE, UIMA_EXC_CLASSIMPLEMENT, ./framework/uima/exceptions.hpp: <TT>UIMA_EXC_CLASSDECLARE</TT> in an .h/.hpp file there must be one use of ./framework/uima/exceptions.hpp:#define UIMA_EXC_CLASSDECLARE(child,parent) \ ./framework/uima/exceptions.hpp: <TT>UIMA_EXC_CLASSDECLARE</TT> (see above) in the corresponding .h/.hpp file ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_logic_error, Exception); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_runtime_error, Exception); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_domain_error, Uima_logic_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_invalid_argument, Uima_logic_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_length_error, Uima_logic_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_out_of_range, Uima_logic_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_range_error, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_overflow_error, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(Uima_underflow_error, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ConsoleAbortExc, Exception); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcDocBuffer, Uima_out_of_range); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ParseHandlerExc, Exception); ./framework/uima/exceptions.hpp://? UIMA_EXC_CLASSDECLARE(ExcAccessError, Uima_runtime_error); ./framework/uima/exceptions.hpp://? UIMA_EXC_CLASSDECLARE(ExcAssertionFailure, Exception); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcIllFormedInputError, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcInvalidParameter, Uima_invalid_argument); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcIndexOutOfRange, Uima_out_of_range); ./framework/uima/exceptions.hpp://? UIMA_EXC_CLASSDECLARE(ExcDeviceError, _runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcInvalidRequest, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcResourceExhausted, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcOutOfMemory, ExcResourceExhausted); ./framework/uima/exceptions.hpp://? UIMA_EXC_CLASSDECLARE(ExcOutOfSystemResource, ResourceExhausted); ./framework/uima/exceptions.hpp://? UIMA_EXC_CLASSDECLARE(ExcOutOfWindowResource, ResourceExhausted); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcFileNotFoundError, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(ExcWinCException, Uima_runtime_error); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(CodePageConversionException, Exception); ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(AprFailureException, Exception); ./framework/uima/exceptions.hpp: To declare a new exception class, use the <TT>UIMA_EXC_CLASSDECLARE</TT> macro in an .h/.hpp ./framework/uima/exceptions.hpp: UIMA_EXC_CLASSDECLARE(AnnotatorException, InvalidRequest); ./framework/uima/casiterator.hpp: UIMA_EXC_CLASSDECLARE(CASIteratorException, uima::Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(UnknownTypeException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(UnknownFeatureException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(UnknownRangeTypeException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(IncompatibleRangeTypeException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(AllowedStringValuesIncompatibleException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(TypePriorityConflictException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(IncompatibleIndexDefinitionsException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(IncompatibleParentTypesException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(CASIncompatibilityException, Exception); ./framework/uima/engine.hpp: UIMA_EXC_CLASSDECLARE(UimaAnalysisComponentException, Exception); ./framework/uima/internal_jedii_engine.hpp: UIMA_EXC_CLASSDECLARE(JavaException, Exception); ./framework/uima/internal_capability_container.hpp: UIMA_EXC_CLASSDECLARE(CapabilityException, Exception); ./framework/uima/taespecifierbuilder.hpp: UIMA_EXC_CLASSDECLARE(InvalidXMLException, uima::Exception); ./framework/uima/caspool.hpp: UIMA_EXC_CLASSDECLARE(CASPoolException, uima::Exception); ./framework/uima/taespecifier.hpp: UIMA_EXC_CLASSDECLARE(ConfigParamLookupException, ConfigException); ./framework/uima/taemetadata.hpp: UIMA_EXC_CLASSDECLARE(DuplicateConfigElemException, uima::Exception); ./framework/uima/taemetadata.hpp: UIMA_EXC_CLASSDECLARE(ValidationException, uima::Exception); ./framework/uima/annotator.hpp: UIMA_EXC_CLASSDECLARE(ExcEnumerationOverflow, ExcIllFormedInputError); ./framework/uima/config_param.hpp: UIMA_EXC_CLASSDECLARE(ConfigException, Exception); ./framework/uima/config_param.hpp: UIMA_EXC_CLASSDECLARE(ConfigParamException, ConfigException);

Would you say UimaAnalysisComponentException is appropriate?

src/framework/annotator_mgr.cpp

src/cas/cas.cpp

DrDub

Still a few questions and comments but overall I managed to go through the whole thing this time.

I'll now proceed to stress test it a bit.

src/test/src/test_engine.cpp

DrDub · 2024-08-22T12:46:27Z

src/test/data/descriptors/SegmentAnnotateMerge.xml

@@ -77,12 +77,12 @@
                    </array>
                </value>
            </nameValuePair>
-            <nameValuePair>


this seems like something didn't need to be committed.

This override would fail the test case for the newer override spec. Do you want me to keep it for now and deal with the test case later, or just remove that portion?

src/framework/annotator_mgr.cpp

DrDub · 2024-08-22T13:19:04Z

src/framework/annotator_mgr.cpp

+          return nullptr;
+        }
+
+        if (iv_bOutputNewCases && !finalStep->getForceDropCAS())


Just to double check I'm understanding the code correctly, this is where the DROP_IF_NEW_CAS_PRODUCED functionality is implemented?

Yes the finalStep will determine if that CAS is dropped or not. In this instance that finalStep is constructed by the FixedFlowController and FixedFlowObject, and the default action in the FixedFlowController is DROP_IF_NEW_CAS_PRODUCED

DrDub · 2024-08-22T13:19:42Z

src/framework/annotator_mgr.cpp

+      if (!result)
+        result = processUntilNextOutputCas();
+      if (!result) {
+        UIMA_EXC_THROW_NEW(Exception,


shouldn't this call also release()?

No when it is returned to the caller/user they will call release themselves. Or do you mean something else?

mac-op and others added 14 commits July 18, 2024 20:19

framework: initial implementation of FlowController and related classes

1232506

framework: allow CAS to keep track of owner and release itself

50ddfb0

test: fixed implementation name in DaveDetector

5d4f657

framework: allow aggregate CAS Multiplier

280b444

TODO: Documentation and exception handling

Merge branch 'apache:main' into feature/agg-cas-multiplier

6d30b20

framework: fixed small hack in AnnotatorManager

8aa5919

No longer have to cast FlowController to FixedFlowController when we want to find an engine

framework: added function and class documentation

d579898

framework: added documentation and comments

0d635ca

Also introduced new Exception: EngineProcessingException

framework: fixed memory leak in AnnotatorManager

0adb05c

FlowController is not deleted on destruction

bug: fixed undefined behavior in Step union

3d4086f

The constructor for the UNSPECIFIED case didn't initialize the union properly, so when SimpleStep's assignment operator is called later on it can segfault when assigning string.

bug: fixed another undefined behavior in Step union

5f7ae16

The union needed to be properly destroyed in assignment operator, and initialized in copy constructor and AO

framework: introduce owner variable for CASes in Pool

f5d045b

clean up for CAS and CASPool

e3f1b33

framework: Added check for invalid call to CAS::release

2034217

This introduces a new error ID and error message UIMA_MSG_ID_EXC_INVALID_CAS_RELEASE

DrDub requested changes Aug 20, 2024

View reviewed changes

mac-op added 4 commits August 20, 2024 13:09

framework: formatting and documentation

d49a58e

framework: correctly setting the CAS Owner in CAS Pool

88ba14a

fix: small bug from last commit in releaseCAS

f89eb88

test: fix incorrect test case for Aggregate CAS combiner

5966b78

DrDub reviewed Aug 22, 2024

View reviewed changes

mac-op added 5 commits August 22, 2024 14:58

test: added test case for Step class

df7bf0c

fix: better exception message in annotator_mgr.cpp

ce9cd6c

fix: added test case in test_engine

7cd9ab9

fix: remove CAS from activeCASes in AnnotatorManager

e07c09f

framework: removed old constructor for CASPool

d4588ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Aggregate CAS Multiplier #25

Support for Aggregate CAS Multiplier #25

mac-op commented Aug 20, 2024 •

edited

Loading

DrDub left a comment

DrDub Aug 20, 2024

DrDub Aug 20, 2024

mac-op Aug 20, 2024

DrDub left a comment

DrDub Aug 22, 2024

mac-op Aug 22, 2024

DrDub Aug 22, 2024

mac-op Aug 22, 2024

DrDub Aug 22, 2024

mac-op Aug 22, 2024

Support for Aggregate CAS Multiplier #25

Are you sure you want to change the base?

Support for Aggregate CAS Multiplier #25

Conversation

mac-op commented Aug 20, 2024 • edited Loading

DrDub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrDub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mac-op commented Aug 20, 2024 •

edited

Loading