Fix manifest parsing #250
Open
Fix manifest parsing #250
Conversation
This file contains 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
| if name == 'all': | ||
| raise ValueError('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.
| if entries is None: | ||
| if name in self._raw_groups: | ||
| return | ||
| self._raw_groups[name] = None | ||
| elif name in self._raw_groups and self._raw_groups[name] is not None: | ||
| raise ValueError(f'a group named {name} was already defined') | ||
| self._raw_groups[name].extend(list(entries) if entries else []) |
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.
mdboom
force-pushed
the
fix-manifest-parsing
branch
from
cbbdc0e
to
b4871bd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.