1655 dataset builder from define xml by DmitryMK · Pull Request #1677 · cdisc-org/cdisc-rules-engine

DmitryMK · 2026-03-31T14:19:28Z

Addressing #1655

Updated 2 builders to keep define.xml information:

VariablesMetadataWithDefineAndLibraryDatasetBuilder
VariablesMetadataWithDefineDatasetBuilder

I have changed VariablesMetadataWithDefineDatasetBuilder to be consistent with the VariablesMetadataWithDefineAndLibraryDatasetBuilder and added a test for this builder:

Added Filling NAs with blanks
Added variable_has_empty_values and variable_is_empty, as logically we might want to run CG0015 without Library metadata, as define.xml has a role attribute and if variable is not in the library/following some standard which is not in the library, we can try to rely on the role values from define.xml instead of the library.

@gerrycampion @SFJohnson24 I have 2 questions:

In VariablesMetadataWithDefineDatasetBuilder I'm populating variable_has_empty_values and variable_is_empty which was not the case before and it might take a significant time on large data and slow down the engine.
When the information is coming from define.xml only, I'm keeping variable_name blank. In the report we can show define_variable_name for such cases. We can fill variable_name with define_variable_name, but I believe in this case we would need to add a new attribute like variable_is_present to identify the cases when a variable is present in define.xml only.
I had to made also some changes in test_rules_engine. I have updated variable name from TEST to TEST2 and added a mock for get_dataset in the test test_validate_variable_metadata_against_define_xml. The mock is needed because of the update in VariablesMetadataWithDefineDatasetBuilder, but I'm not sure if TEST was intentional variable name before at this line.

…er consistent with dataset+define+library bilder.

…th define.xml

SFJohnson24 · 2026-04-30T20:50:42Z

@DmitryMK there is a merge conflict--can you resolve it so I can review.

RamilCDISC

Could you please resolve the conflict on the branch too?

RamilCDISC · 2026-05-01T19:22:55Z

            right_on="define_variable_name",
-            how="left",
+            how="outer",
+        ).fillna("")


If we make the Nan's a empty string "" we lose information if the columns was missing or it is just empty. I think better would be to handle this in a different way. Please let me know if I am not understanding the real intent here.

Hi Ramil, I did it for consistency with metadata+define+library dataset builder, as it had fillna("") in it.

What is the difference between missing and empty column? In my understanding it either comes from dataset(metadata) or define. So the missing values logically appear only when it is present in define only. I might be wrong here.

DmitryMK · 2026-05-04T15:17:51Z

@SFJohnson24 @RamilCDISC I have merged the main branch and updated the tests accordingly.
I have one of the checks failing. It does not looks like it is caused by this PR. Do you know what it can be connected with.

gerrycampion · 2026-05-07T17:46:51Z

@SFJohnson24 @RamilCDISC I have merged the main branch and updated the tests accordingly. I have one of the checks failing. It does not looks like it is caused by this PR. Do you know what it can be connected with.

@DmitryMK this issue is resolved now

RamilCDISC

For manual validation I ran the rule in the connected ticket description against negative dataset in the Sharepoint. The builder had an error. The trace is below.

python core.py validate -s sdtmig -v 3.4 -lr rule.yml -dp "/Users/ramilabbasov/Library/CloudStorage/OneDrive-CDISC/Documents - CORE Rules/unitTesting/SDTMIG/CG0015/negative/01/data/unit-test-coreid-CG0015-negative1.xlsx" -dxp "/Users/ramilabbasov/Library/CloudStorage/OneDrive-CDISC/Documents - CORE Rules/unitTesting/SDTMIG/CG0015/negative/01/data/define_CG0015-negative.xml"
Traceback (most recent call last):
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/parsers/base_parser.py", line 838, in _cast_types
    values = astype_array(values, cast_type, copy=True)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/core/dtypes/astype.py", line 183, in astype_array
    values = _astype_nansafe(values, dtype, copy=copy)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/core/dtypes/astype.py", line 134, in _astype_nansafe
    return arr.astype(dtype, copy=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: could not convert string to float: '2021-04-27'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/core.py", line 1094, in <module>
    cli()
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/core.py", line 669, in validate
    run_validation(
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/scripts/run_validation.py", line 180, in run_validation
    ).get_data_service(args.dataset_paths)
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/cdisc_rules_engine/services/data_services/data_service_factory.py", line 89, in get_data_service
    return self.get_service(
           ^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/cdisc_rules_engine/services/data_services/data_service_factory.py", line 147, in get_service
    return self._registered_services_map.get(service_name).get_instance(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/cdisc_rules_engine/services/data_services/excel_data_service.py", line 51, in get_instance
    service = cls(
              ^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/cdisc_rules_engine/services/data_services/excel_data_service.py", line 39, in __init__
    super(ExcelDataService, self).__init__(
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/cdisc_rules_engine/services/data_services/base_data_service.py", line 120, in __init__
    self._initialize_datasets_metadata(**kwargs)
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/cdisc_rules_engine/services/data_services/excel_data_service.py", line 178, in _initialize_datasets_metadata
    dataset = self.__get_dataset(dataset_filename)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/ISS-1655/cdisc-rules-engine/cdisc_rules_engine/services/data_services/excel_data_service.py", line 96, in __get_dataset
    dataframe = pd.read_excel(
                ^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/excel/_base.py", line 517, in read_excel
    data = io.parse(
           ^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/excel/_base.py", line 1629, in parse
    return self._reader.parse(
           ^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/excel/_base.py", line 931, in parse
    raise err
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/excel/_base.py", line 918, in parse
    output[asheetname] = parser.read(nrows=nrows)
                         ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 1748, in read
    ) = self._engine.read(  # type: ignore[attr-defined]
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/parsers/python_parser.py", line 289, in read
    conv_data = self._convert_data(data)
                ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/parsers/python_parser.py", line 360, in _convert_data
    return self._convert_to_ndarrays(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/parsers/base_parser.py", line 593, in _convert_to_ndarrays
    cvals = self._cast_types(cvals, cast_type, c)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ramilabbasov/Desktop/ISSUES/venv/lib/python3.12/site-packages/pandas/io/parsers/base_parser.py", line 840, in _cast_types
    raise ValueError(
ValueError: Unable to convert column SVENDTC to type float64 (sheet: sv.xpt)
((venv) ) ramilabbasov@Ramils-MacBook-Pro cdisc-rules-engine %

DmitryMK added 2 commits March 31, 2026 09:57

Using outer join for define.xml metadata. Making dataset+define build…

1a1dbb4

…er consistent with dataset+define+library bilder.

Updating the test, as now contents is read when building a dataset wi…

ec3d65b

…th define.xml

DmitryMK temporarily deployed to DEV March 31, 2026 14:19 — with GitHub Actions Inactive

DmitryMK requested a review from SFJohnson24 March 31, 2026 14:19

Formatting with black

9ebe4c6

DmitryMK temporarily deployed to DEV March 31, 2026 14:50 — with GitHub Actions Inactive

SFJohnson24 requested a review from gerrycampion March 31, 2026 16:39

gerrycampion linked an issue Apr 27, 2026 that may be closed by this pull request

Allow Rule Types that include define-xml to start the dataset builder from define-xml (and not only from dataset). #1655

Open

RamilCDISC requested changes May 1, 2026

View reviewed changes

Merging the main branch. Resolving merge conflict

508efb8

DmitryMK temporarily deployed to DEV May 4, 2026 12:28 — with GitHub Actions Inactive

Updated the new test according to the changes in the main branch

6033b20

DmitryMK temporarily deployed to DEV May 4, 2026 15:03 — with GitHub Actions Inactive

Merge branch 'main' into 1655-dataset-builder-from-define-xml

9cbe5b9

gerrycampion temporarily deployed to DEV May 4, 2026 17:37 — with GitHub Actions Inactive

Merge branch 'main' into 1655-dataset-builder-from-define-xml

62a41e0

gerrycampion temporarily deployed to DEV May 4, 2026 18:00 — with GitHub Actions Inactive

gerrycampion mentioned this pull request May 5, 2026

Change distinct sets to lists #1721

Merged

Merge branch 'main' into 1655-dataset-builder-from-define-xml

9aaeb8a

gerrycampion temporarily deployed to DEV May 7, 2026 17:27 — with GitHub Actions Inactive

RamilCDISC temporarily deployed to DEV May 10, 2026 20:39 — with GitHub Actions Inactive

RamilCDISC requested changes May 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1655 dataset builder from define xml#1677

1655 dataset builder from define xml#1677
DmitryMK wants to merge 8 commits intomainfrom
1655-dataset-builder-from-define-xml

DmitryMK commented Mar 31, 2026

Uh oh!

SFJohnson24 commented Apr 30, 2026

Uh oh!

RamilCDISC left a comment

Uh oh!

RamilCDISC May 1, 2026

Uh oh!

DmitryMK May 1, 2026

Uh oh!

DmitryMK commented May 4, 2026

Uh oh!

gerrycampion commented May 7, 2026

Uh oh!

RamilCDISC left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

DmitryMK commented Mar 31, 2026

Uh oh!

SFJohnson24 commented Apr 30, 2026

Uh oh!

RamilCDISC left a comment

Choose a reason for hiding this comment

Uh oh!

RamilCDISC May 1, 2026

Choose a reason for hiding this comment

Uh oh!

DmitryMK May 1, 2026

Choose a reason for hiding this comment

Uh oh!

DmitryMK commented May 4, 2026

Uh oh!

gerrycampion commented May 7, 2026

Uh oh!

RamilCDISC left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants