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.
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
Differential graphs on report #657
Differential graphs on report #657
Changes from all commits
1ba9bc1
c800394
7edd98a
5c790d9
3cc5904
5303158
f334a61
295d84a
dca1a89
90db758
6f85328
eedffec
8869dca
024bc4d
a8924ac
0cba1cb
14a910a
3852e58
b7d2e69
aa232a2
b68f970
b90f4ae
c702c35
adb072b
62307cb
50f9acc
039857e
0981067
60f3585
95f9146
195585f
6ffb6c9
79a86a5
f6059bc
6906a96
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-( we don't make gsutil a dependency for fuzzbench. This method of downloading the data over http allows us not to do this but doesn't support a unix filesystem as a filestore. @lszekeres thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think my understanding of what this is used for is wrong.
@Shadoom7 is this function only used for displaying a link in a report?
Maybe we can implement some of this as a helper function filestore utils that just assumes the http link and we can refactor later when we actually support local experiments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's only used to display the link. Do you mean we should add the function in
filestore_utils
to return the http link and we just call that function here, ignoring the case where filestore_path is local?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put it into
filestore_utils
though? I thoughtfilestore_utils
only contains possible terminal commands that are related to filestore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Jonathan's original question is more relevant to line 63 of
coverage_data_utils.py
(filestore_utils.cp(src_file, dst_file)
), which copies the JSON file first with gsutil cp or with cp. If we want to eliminategenerate_report.py
's dependency ongstuil
(which would be a good idea), what we could do is:wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah can come later but it is probably a good idea. I sort of wished we investigated alternatives to gsutil more since this is another edge case we have to deal with on our own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to work with merging experiments right?
Can you confirm that it does? Looking at this function, it feels like intuitively it does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I'll test it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I guess this sort of assumes that benchmarks for a fuzzer all come from the same experiment. I don't think we make that assumption elsewhere though. I think the assumption is fine to make though it might break in some rare cases (such as when we add benchmarks). Can you at least document this assumption please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested and it works. The
benchmark_df
here is the dataframe for one certain benchmark. So we are not assuming all benchmarks for a fuzzer come from the same experiment. We are just assuming all trials for a certain pair of fuzzer-benchmark come from the same experiment, which is whatmerge_with_clobber
does.