Skip to content

Detect cycles in depends_on - #254

Merged
jl-wynen merged 3 commits into
mainfrom
depect-depends_on-loops
Nov 25, 2024
Merged

jl-wynen merged 3 commits into
mainfrom
depect-depends_on-loops

Conversation

@jl-wynen

Copy link
Copy Markdown
Member

We have integration tests with broken files that have circular depends_on graphs. ScippNeXus currently runs in an infinite loop in those cases.

This PR adds

  • cycles in depends_on chains are detected and lead to ValueErrors.
  • Self-referencing depends_on are treated as depends_on == '.'. While this does not comply with the standard, I see no reason not to do this.

@jl-wynen
jl-wynen requested a review from nvaytet November 25, 2024 12:57

root = make_group(h5root)
with pytest.raises(ValueError, match="Circular depends_on"):
_ = root[()]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks.

Did you try loading a NMX file from coda to check that the error is raised?

@jl-wynen jl-wynen Nov 25, 2024 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did now and it does find a cycle:

'/entry/instrument/detector_panel_0/transformations/orientation',
'/entry/instrument/detector_panel_0/transformations/axis6',
'/entry/instrument/detector_panel_0/transformations/axis5',
'/entry/instrument/detector_panel_0/transformations/axis4',
'/entry/instrument/detector_panel_0/transformations/axis3',
'/entry/instrument/detector_panel_0/transformations/axis2',
'/entry/instrument/detector_panel_0/transformations/axis1'
'/entry/instrument/detector_panel_0/transformations/stageZ'
'/entry/instrument/detector_panel_0/transformations/orientation'

@jl-wynen
jl-wynen merged commit bfc0b63 into main Nov 25, 2024
@jl-wynen
jl-wynen deleted the depect-depends_on-loops branch November 25, 2024 15:25
@SimonHeybrock

Copy link
Copy Markdown
Member
* Self-referencing `depends_on` are treated as `depends_on == '.'`. While this does not comply with the standard, I see no reason not to do this.

Can you explain why you think this is a good approach? I thought it would be easy to misjudge a typo for a chain-end, i.e., say trans1.attrs['depends_on'] = 'trans1', a typo of 'trans2'. I think we should treat this an error.

But maybe I misunderstood what you meant?

@jl-wynen

Copy link
Copy Markdown
Member Author

This is what I meant. I don't have a strong opinion on this. But I figured that since we treat depends_on as posix paths, we could make it more consistent. In particular if writers also treat them as paths.
I don't think that typos are a big concern. How many files are written by hand?

@SimonHeybrock

Copy link
Copy Markdown
Member

I don't think that typos are a big concern. How many files are written by hand?

I guess close to 100% of the code or config files used to write files is written by hand. I am not sure what you are saying. I think self-loops are just as bad as other loops?

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.

3 participants