Fix manifest parsing#250
Open
Fix manifest parsing #250
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
ericsnowcurrently requested changes Dec 3, 2022
| ifname=='all': | ||
| raiseValueError('a group named "all" is not allowed ("all" is reserved for selecting the full set of declared benchmarks)') |
There was a problem hiding this comment.
This should still be the case. So the Pyston benchmarks' manifest needs to be fixed.
| self._raw_groups[name] =None | ||
| elifnameinself._raw_groupsandself._raw_groups[name] isnotNone: | ||
| raiseValueError(f'a group named {name} was already defined') | ||
| self._raw_groups[name].extend(list(entries) ifentrieselse []) |
There was a problem hiding this comment.
This rule should apply for a single manifest, with the exception only for includes (which should be merged like you are doing). I expect the logic for distinguishing between the two cases is lacking, either in `_parse_manifest() below or in the code that consumes its output.
mdboomforce-pushed the fix-manifest-parsing branch from cbbdc0e to b4871bdCompareSign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm a little unclear about how the manifest parsing is supposed to work, so I'm not sure this is the correct solution.
To reproduce the bug, use a "meta" manifest file that loads both the pyperformance suite and the pyston python-macrobenchmarks suite (as we do in our infra for the faster-cpython project):
There seems to be two bugs:
Both pyperformance and pyston have an empty
[group default]entry. I'm not 100% sure what that is supposed to do in practice, but it causes the "a group named default was already defined" error to raise. Here, I just make the groups concatenate instead.The pyston manifest has:
It seems like this should be valid, but the code here disallows any mention of
all.Again, I have no idea if this fix is correct, but it does at least let these two suites of benchmarks combine correctly.