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

Better view analysis #264

Merged
merged 5 commits into from
Mar 27, 2023
Merged

Better view analysis #264

merged 5 commits into from
Mar 27, 2023

Conversation

EdwardLarson
Copy link
Collaborator

One sentence summary of this PR (This should go in the CHANGELOG!)
Choose Analyzer components which output the entirety of a view, rather than piece by piece, which would choose the wrong Analyzer sometimes.

Link to Related Issue(s)
ResourceViews are composed of multiple attributes types, and when running something like view_as OFRAK would previously make a request for each of those attributes types. With limited visibility into what the user actually wants, the job service would sometimes end up choosing Analyzers which do output one or more of the requested attributes but actually require the analysis of the other attributes, resulting in a deadlock.

Please describe the changes in your request.

  • Auto-analyze attributes requests now can take multiple attributes types at once (and view_as uses this)
  • The component filter for analyzer output looks for analyzers which output all of the requested attributes (and possibly other attributes as well), instead of any analyzers which output one or more of the requested attributes
  • When auto-analyzing attributes, the job service first looks for an Analyzer outputting all of the requested attributes, and if such an Analyzer is not found, falls back to looking for Analyzers for each requested attribute individually (the previous behavior)

Anyone you think should look at this, specifically?
@rbs-jacob

@rbs-jacob
Copy link
Member

I'm a little confused about whether this reliably solves the problem based on this last bullet:

  • When auto-analyzing attributes, the job service first looks for an Analyzer outputting all of the requested attributes, and if such an Analyzer is not found, falls back to looking for Analyzers for each requested attribute individually (the previous behavior)

This makes it sound like there is still a risk of deadlock in certain circumstances?

@EdwardLarson
Copy link
Collaborator Author

This makes it sound like there is still a risk of deadlock in certain circumstances?

That's correct. The true solution to the root cause would be pretty complicated. The approach I have in mind is to flesh out the job context to include a "stack" of the components or at least analyzers being run, and raise an error when the same analyzer appears twice in the stack. However that would take me a couple days at least to do, this took me an hour or so.

And besides this does solve a real problem: The wrong analyzer was being chosen in a test that was deadlocking. The job service was choosing the ComplexBlockSymbolAnalyzer for one of the attributes in a ComplexBlock view, and using the ComplexBlockAnalyzer for other attributes. Since the former indirectly calls the latter, the set of analysis tasks was deadlocking.

@rbs-jacob
Copy link
Member

Thanks for the clarification!

Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

Two very small changes, and this should be good to go!

ofrak_core/ofrak/resource.py Outdated Show resolved Hide resolved
ofrak_core/ofrak/service/job_service.py Outdated Show resolved Hide resolved
@EdwardLarson EdwardLarson merged commit 059f88e into master Mar 27, 2023
@whyitfor whyitfor deleted the feature/better_view_analysis branch April 10, 2023 19:45
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.

None yet

3 participants