FossID: Limit fossid snippets #8616

nnobelis · 2024-05-03T13:45:27Z

Please have a look at the individual commit messages for the details.

codecov · 2024-05-03T13:55:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.74%. Comparing base (76237c2) to head (5c336a3).
Report is 35 commits behind head on main.

❗ Current head 5c336a3 differs from pull request most recent head 08c9e38. Consider uploading reports for the commit 08c9e38 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8616      +/-   ##
============================================
- Coverage     67.97%   67.74%   -0.23%     
  Complexity     1005     1005              
============================================
  Files           244      244              
  Lines          7844     7844              
  Branches        876      876              
============================================
- Hits           5332     5314      -18     
- Misses         2129     2147      +18     
  Partials        383      383

Flag	Coverage Δ
funTest-docker	`66.04% <ø> (-0.45%)`	⬇️
funTest-non-docker	`34.79% <ø> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

plugins/scanners/fossid/src/main/kotlin/FossIdConfig.kt

plugins/scanners/fossid/src/main/kotlin/FossId.kt

plugins/scanners/fossid/src/main/kotlin/FossIdScanResults.kt

plugins/scanners/fossid/src/main/kotlin/FossIdConfig.kt

plugins/scanners/fossid/src/main/kotlin/FossId.kt

plugins/scanners/fossid/src/main/kotlin/FossIdScanResults.kt

Signed-off-by: Nicolas Nobelis <[email protected]>

This property allows to control the amount of snippets to query from the FossID instance. Indeed, some repositories have a huge amount of snippets (> 400k), and querying them all, putting them in the ORT results and displaying them in the snippet report has a huge performance impact. It also does not make lots of sense on the functional level. This property has a default value of 500 snippets. Signed-off-by: Nicolas Nobelis <[email protected]>

Clarified

oheger-bosch · 2024-05-10T06:38:26Z

plugins/scanners/fossid/src/main/kotlin/FossId.kt

- val snippets = runBlocking(Dispatchers.IO) {
- pendingFiles.map {
- async {
+ val snippets = pendingFiles.associateWith {


The new implementation is now fully sequential; every concurrent processing of files has been turned off. Could this be a performance issue if there are many pending files?

This is correct. Let's take an example with n pending files and s snippets in total.

Before the changes, one request was made to list the snippets per pending file, so n requests in total. Then, one request to list the matched lines per snippet. Therefore n + s in total, done concurrently.

With the changes, there is now n + min(s, limit) requests in total, done sequentially,

I would assume this is less efficient but this is nevertheless a huge improvement for diminishing the amount of requests, as soon as the amount of snippet is growing.

Unfortunately, with the introduction of lazy loading, the initial concurrency cannot be get back. However, all the matched lines for all snippets of one pending file can be made concurrently ?

Any suggestion ?

My guts feeling is n << s, by a factor 10 at least.

I put again the code to list snippets of a pending file concurrently.

As we discussed, the proper solution to avoid all these problem would be to put the listing and mapping code together.
However, this is a big refactoring and I worry that the snippet choice feature relies on having all snippets information at once for a given pending file.

Additionally, it seems FossID is moving toward OpenAPI to replace the legacy API, hence I would like to wait for their new release first, before doing such a refactoring.

oheger-bosch · 2024-05-10T06:40:38Z

plugins/scanners/fossid/src/main/kotlin/FossId.kt

- async {
+ val snippets = pendingFiles.associateWith {
+ val filteredSnippets = lazy {
+ runBlocking(Dispatchers.IO) {


Since getRawResults() is a suspend function, it should not call runBlocking(). Use withContext(Dispatchers.IO) instead.

Thanks for the hint. Unfortunately, withContext is a suspending function and it cannot be used with lazy, because the latter does not support suspension.

To avoid runBlocking altogether, I refactored the code to generate a Sequence as @sschuberth suggested.
Unfortunately, the function building a Sequence cannot be suspendable.
Therefore, I went with a Flow: values are emitted asynchronously and if the limit has been reached, the collecting job is cancelled.

plugins/scanners/fossid/src/main/kotlin/FossId.kt

plugins/scanners/fossid/src/main/kotlin/FossIdScanResults.kt

plugins/scanners/fossid/src/main/kotlin/FossId.kt

oheger-bosch

Looks good to me. Any objections from anybody against merging this?

sschuberth · 2024-05-14T14:41:14Z

Any objections from anybody against merging this?

There's a unit test failure now. Please first have a look at this.

Currently, the FossID snippets are all listed in a first step, including their matching lines. Then in a second step, they are mapped to ORT snippets. A future commit is going to introduce a limit for listing snippets. If this limit is applied to the current logic (i.e. when listing the snippets), it is going to clash with the snippet choice feature: chosen snippets are removed from the results and thus should not be counted when enforcing the limit. Therefore, this commit makes the listed snippets a Flow. When mapping the snippets and applying the snippet choices, the current snippet count can then be compared to the limit and additional snippets can lazily be fetched. Signed-off-by: Nicolas Nobelis <[email protected]>

Since the function `mapSnippetFindings` is going to increase in complexity in a future commit, with the enforcing of the snippets limit, extract this function now. Signed-off-by: Nicolas Nobelis <[email protected]>

The limit is enforced when mapping the FossID snippets to ORT snippets. Signed-off-by: Nicolas Nobelis <[email protected]>

…ched Also display this issue in the FossID snippet report. In case the limit has been exactly reached when listing the snippets of a pending file, adding an issue is the only possibility for the user to know that there may be other snippets beyond the limit, requiring some snippet choices and a second scanner run. A strict comparison could be used instead but deciding to create the issue would then require peeking the next snippet results, which would defeat the optimization brought by the introduction of the limit. Signed-off-by: Nicolas Nobelis <[email protected]>

When the usage of the Flow was introduced, snippets were listed sequentially for a given file. This commit adds concurrent processing again. Signed-off-by: Nicolas Nobelis <[email protected]>

oheger-bosch · 2024-05-15T11:48:11Z

@sschuberth, are you fine with merging?

sschuberth · 2024-05-15T11:52:18Z

@sschuberth, are you fine with merging?

I haven't looked at the latest iterations code-wise, and I'm not deep enough into the topic to do so in reasonable time. So I don't feel comfortable to formally approve, but if you've approved and all tests pass (the Docker Build failure seems unrelated, GitHub apparently just had some problems today), I don't see a reason not to merge. Please go ahead as you see fit.

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch 2 times, most recently from f54a42d to e313a9e Compare May 6, 2024 06:32

nnobelis marked this pull request as ready for review May 6, 2024 06:55

nnobelis requested a review from a team as a code owner May 6, 2024 06:55

sschuberth reviewed May 6, 2024

View reviewed changes

plugins/scanners/fossid/src/main/kotlin/FossIdConfig.kt Show resolved Hide resolved

plugins/scanners/fossid/src/main/kotlin/FossId.kt Outdated Show resolved Hide resolved

plugins/scanners/fossid/src/main/kotlin/FossIdScanResults.kt Outdated Show resolved Hide resolved

fviernau previously requested changes May 6, 2024

View reviewed changes

nnobelis added 2 commits May 8, 2024 08:26

chore(fossid-webapp): Align the configuration property names

b66785c

Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch from e313a9e to 5792c9f Compare May 8, 2024 06:48

nnobelis requested review from fviernau and sschuberth May 8, 2024 06:48

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch from 5792c9f to af9b0a2 Compare May 8, 2024 12:05

nnobelis requested a review from oheger-bosch May 8, 2024 12:09

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch from af9b0a2 to 5d9ea6b Compare May 8, 2024 17:50

oheger-bosch reviewed May 10, 2024

View reviewed changes

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch from 5d9ea6b to a9811b0 Compare May 13, 2024 12:10

nnobelis requested a review from oheger-bosch May 13, 2024 12:26

oheger-bosch reviewed May 13, 2024

View reviewed changes

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch from a9811b0 to d870f59 Compare May 14, 2024 06:52

nnobelis requested a review from oheger-bosch May 14, 2024 06:54

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch 2 times, most recently from d7fea61 to 5c336a3 Compare May 14, 2024 11:00

oheger-bosch previously approved these changes May 14, 2024

View reviewed changes

nnobelis added 4 commits May 15, 2024 09:36

chore(fossid-webapp): Extract the function mapSnippetFindingsForFile

d09013c

Since the function `mapSnippetFindings` is going to increase in complexity in a future commit, with the enforcing of the snippets limit, extract this function now. Signed-off-by: Nicolas Nobelis <[email protected]>

feat(fossid-webapp): Enforce the snippets limit

0efdba9

The limit is enforced when mapping the FossID snippets to ORT snippets. Signed-off-by: Nicolas Nobelis <[email protected]>

fix(fossid-webapp): List snippets concurrently

08c9e38

When the usage of the Flow was introduced, snippets were listed sequentially for a given file. This commit adds concurrent processing again. Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis dismissed oheger-bosch’s stale review via 08c9e38 May 15, 2024 07:36

nnobelis force-pushed the nnobelis/limit_fossid_snippets branch from 5c336a3 to 08c9e38 Compare May 15, 2024 07:36

sschuberth requested a review from oheger-bosch May 15, 2024 08:12

oheger-bosch approved these changes May 15, 2024

View reviewed changes

oheger-bosch merged commit b793e22 into oss-review-toolkit:main May 15, 2024
17 of 18 checks passed

oheger-bosch deleted the nnobelis/limit_fossid_snippets branch May 15, 2024 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FossID: Limit fossid snippets #8616

FossID: Limit fossid snippets #8616

nnobelis commented May 3, 2024

codecov bot commented May 3, 2024 •

edited

oheger-bosch May 10, 2024

nnobelis May 13, 2024

nnobelis May 13, 2024 •

edited

nnobelis May 13, 2024

oheger-bosch May 10, 2024

nnobelis May 13, 2024

nnobelis May 13, 2024

oheger-bosch left a comment

sschuberth commented May 14, 2024

oheger-bosch commented May 15, 2024

sschuberth commented May 15, 2024

FossID: Limit fossid snippets #8616

FossID: Limit fossid snippets #8616

Conversation

nnobelis commented May 3, 2024

codecov bot commented May 3, 2024 • edited

Codecov Report

oheger-bosch May 10, 2024

Choose a reason for hiding this comment

nnobelis May 13, 2024

Choose a reason for hiding this comment

nnobelis May 13, 2024 • edited

Choose a reason for hiding this comment

nnobelis May 13, 2024

Choose a reason for hiding this comment

oheger-bosch May 10, 2024

Choose a reason for hiding this comment

nnobelis May 13, 2024

Choose a reason for hiding this comment

nnobelis May 13, 2024

Choose a reason for hiding this comment

oheger-bosch left a comment

Choose a reason for hiding this comment

sschuberth commented May 14, 2024

oheger-bosch commented May 15, 2024

sschuberth commented May 15, 2024

codecov bot commented May 3, 2024 •

edited

nnobelis May 13, 2024 •

edited