Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: include type alias source pragmas in db generation #311

Merged
merged 2 commits into from
Jun 20, 2023

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Jun 15, 2023

Without this PR, type aliases don't "see" the pragmas from their type source.
This means the database is generated with large numbers of missing records if we keep using the old names (DUT_MotionStage, DUT_PositionState)
Database generation diff: pcdshub/lcls-plc-example-motion@6bbae6b

@ZLLentz ZLLentz requested review from klauer and tangkong June 15, 2023 21:43
@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 15, 2023

Sorry, this definitely broke "something"- I should have slowed down before submitting. My new test passes but I forgot to check the rest of the long test suite.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 16, 2023

More detail: something I'm doing has managed to create unexpected bonus records. I'll dive deeper into it soon since this PR is definitely needed prior to deploying the ND motion thing.

_____________________________ test_allow_no_pragma _____________________________

    def test_allow_no_pragma():
        """
        Test for the existence of a variable included in the records, despite it
        lacking a proper set of pragmas.
        """
        tmc_file_name = TMC_ROOT / ("xtes_sxr_plc.tmc")
    
        tmc = parser.parse(tmc_file_name)
    
        records, exceptions = db_process(
            tmc,
            dbd_file=None,
            allow_errors=True,
            show_error_context=True,
            allow_no_pragma=False,
        )
    
        all_records, exceptions = db_process(
            tmc,
            dbd_file=None,
            allow_errors=True,
            show_error_context=True,
            allow_no_pragma=True,
        )
        good_records = 129
        total_records = 1002
    
        assert good_records == len(records)
        assert good_records == len(list(x.valid for x in records if x.valid))
>       assert total_records == len(all_records)
E       AssertionError: assert 1002 == 1005
E        +  where 1005 = len([RecordPackage(tcname='.TCPADS_MAXUDP_BUFFSIZE'), RecordPackage(tcname='Constants.CompilerVersion.uiMajor'), RecordPackage(tcname='Constants.CompilerVersion.uiMinor'), RecordPackage(tcname='Constants.CompilerVersion.uiPatch'), RecordPackage(tcname='Constants.CompilerVersion.uiServicePack'), RecordPackage(tcname='Constants.CompilerVersionNumeric'), ...])

@klauer
Copy link
Contributor

klauer commented Jun 19, 2023

What are the new records it finds?

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 19, 2023

What actually happens here is that the library finds 3 new record candidates in the test case. These are all of type AMSNETID which, has BYTE as its base type. Since we now check base types, and since BYTE is something we could make a record of, it is now found.
image

@klauer
Copy link
Contributor

klauer commented Jun 19, 2023

Seems like a bug - though I'm curious what our IOC databases will look like after this PR. Any change?

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 19, 2023

Note that this test makes 3 new records only when allow_no_pragma=True, not for the normal case.

Personally, my database didn't have any weird bonus records other than reinstating the PVs for DUT_MotionStage, etc.
I didn't accumulate any AMSNETID records because none of mine are pragma'd (I think?)

Two tests I want to run:

  1. Run pytmc tagged vs this PR on a few real projects
  2. Include an AMSNETID variable with a pragma to see what happens before and after this diff

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 20, 2023

I did test 1 for all of the production beamline plcs that I could quickly remember, provided pytmc master branch ran without errors. This excludes lfe and kfe vac which do not build iocs right now- I will make an issue and investigate separately.

For all the others, this did not change their record generation at all. (Note: no production plc is running the unreleased motion lib that turns DUT_MotionStage into an alias.)

You can snoop on my results at /cds/home/z/zlentz/temp/pytmc_testing/pytmc_testing.diff and in that folder for my quick/messy metholodoly

Copy link
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the additional info, this seems good to me 👍

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 20, 2023

Test 2 had more mixed results.

I created an AMSNETID instance in lcls-plc-example-motion and added a pragma (pcdshub/lcls-plc-example-motion@7638617)

Building with pytmc master did not add any PVs pointing to this, which makes sense given what we see in the test suite.
Building with this PR finds the symbol and tries to make a record but gets stuck:

ERROR:pytmc.bin.template:Failed to create EPICS records
Traceback (most recent call last):
  File "/cds/home/z/zlentz/pydev/pytmc/bin/template.py", line 374, in get_plc_record_packages
    packages, exceptions = db_process(
  File "/cds/home/z/zlentz/pydev/pytmc/bin/db.py", line 154, in process
    record_names = [
  File "/cds/home/z/zlentz/pydev/pytmc/bin/db.py", line 158, in <listcomp>
    for single_record in record_package.records
  File "/cds/home/z/zlentz/pydev/pytmc/record.py", line 619, in records
    records = [self.generate_input_record()]
  File "/cds/home/z/zlentz/pydev/pytmc/record.py", line 955, in generate_input_record
    record = super().generate_input_record()
  File "/cds/home/z/zlentz/pydev/pytmc/record.py", line 511, in generate_input_record
    record.fields["DTYP"] = self.dtyp
  File "/cds/home/z/zlentz/pydev/pytmc/record.py", line 952, in dtyp
    return data_types[self.chain.data_type.name]
KeyError: 'AMSNETID'
Traceback (most recent call last):
  File "/cds/group/pcds/pyps/conda/py39/envs/pcds-5.7.2/bin/pytmc", line 10, in <module>
    sys.exit(main())
  File "/cds/home/z/zlentz/pydev/pytmc/bin/pytmc.py", line 106, in main
    func(**kwargs)
  File "/cds/home/z/zlentz/pydev/pytmc/bin/template.py", line 783, in main
    raise stashed_exception
  File "/cds/home/z/zlentz/pydev/pytmc/bin/template.py", line 776, in main
    rendered = render_template(template_text, template_args)
  File "/cds/home/z/zlentz/pydev/pytmc/bin/template.py", line 660, in render_template
    return env.get_template("template").render(context)
  File "/cds/group/pcds/pyps/conda/py39/envs/pcds-5.7.2/lib/python3.9/site-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/cds/group/pcds/pyps/conda/py39/envs/pcds-5.7.2/lib/python3.9/site-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "<template>", line 46, in top-level template code
  File "/cds/home/z/zlentz/pydev/pytmc/bin/template.py", line 525, in generate_records
    db_string = "\n\n".join(package.render() or "" for package in packages)
TypeError: 'NoneType' object is not iterable

My intuition based on the traceback is that the type alias handling I added for DUT_MotionStage actually needs to be done automatically by inspecting the BaseType flag to truly finish the work here. Maybe that belongs in a separate PR though.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 20, 2023

I think the "correct" behavior would be:

  • link AMSNETID to ARRAY[0..5] OF BYTE because it has a BaseType element linked to BYTE and a corresponding ArrayInfo
  • generate a char waveform record with 6 elements

The bug here is that types aliased from basic types that were previously ignored by pytmc will now make errors instead, which is almost certainly a bug but doesn't seem fatal.

@klauer
Copy link
Contributor

klauer commented Jun 20, 2023

I think in this specific case making AMSNETID determine that it's just a byte array wouldn't be unreasonable. That being said a more useful conversion would be to support AMSNETID directly in the IOC where it would auto-convert to a string A.B.C.D.E.F.

So to that end I'm not sure we want "use the base type" behavior in general... My current feeling is that it's worth going with what you have for starters and revisiting.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 20, 2023

You're absolutely right, in this specific case the byte array would be unwieldy.
There are other ams net id types that have STRING basetypes though, so I don't think making a special case for the byte array version is the solution either. I think we'd end up pragma-ing one of those and expecting the string to pass through. (e.g. https://github.com/pcdshub/lcls-plc-example-motion/blob/3c9a0681702283bfe79b4211a3f73e063401e2b8/lcls-twincat-motion-example/tc_mot_example/tc_mot_example.tmc#L3526-L3531)

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 20, 2023

I think what I want to do is:

  • merge
  • create an issue with a link to this discussion
  • solve the docs issue that is holding up CI on this repo
  • return to some of my other to-do items and revisit pytmc later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants