Virtual Codebase modification of the determine deltas by pratik0316 · Pull Request #167 · aboutcode-org/deltacode

pratik0316 · 2021-04-04T05:36:40Z

The goal of this pull request is to refactor DeltaCode to use Scancode-Toolkit’s Virtual Codebase class. This refactoring will
allow DeltaCode to be a library as opposed to only be used as a CLI tool, moreover this refactor will allow DeltaCode to
determine deltas much more effectively in the form of BFS tree scan of the two tree structures unlike indexing the entire
codebase.

In the scanCode Toolkit we can recreate a Codebase object and its
Resource(file, dirs) from a scancode json results file.

So using Virtual Codebase we can leverage upon its Resource class instead of using separate models for Files and Directories.

Since now we are relying on BFS scans of the two tree liked codebases we can get rid of indexing the entire codebase at once, instead of it we yield the resources which are required for the time being.

Signed-off-by: Pratik Dey <pratikrocks.dey11@gmail.com>

pratik0316 · 2021-04-04T05:40:00Z

+        new_visited = 0
+        old_visited = 0
+        old_files_sha1_considered_in_deltas = dict()



@MaJuRG changed the determine delta to the approach we discussed. You meant to implement in this way right?

pratik0316 · 2021-04-04T05:45:26Z

At present, there will be many test cases that are bound to fail as I removed several utilities which will no longer be required, and eventually, those test cases will be removed.
The test cases which I really concern about is the tests cases which deals with checking the delta statuses , delta score, checking delta factors, license / copyrignt extraction, count deltas and many more (those are test in test_deltacode.py file).
Those test cases are absolutely working as per expected.
The failing test cases are for the removed dependencies.

steven-esser · 2021-04-10T01:27:16Z

+        self.new_files_fingerprint = dict() # map of { {new_file1:fingerprint},{new_file2:fingerprint},...} it will be needed when we need the fingerprints
+        self.old_files_fingerprint = dict() # map of { {old_file1:fingerprint},{old_file2:fingerprint},...}
+        self.new_files_original_path = dict() #this keeps a map of the path of file with respect to original path 


I think we can remove the fingerprints dicts here once your PR in common code is merged.

Can you explain why new_files_original_path is necessary?

yes.

Consider the new_codebase files as new/a1.py , new/a2.py and the old_codebasestructure as old/a1.py , old/a4.py .
Now what alignscan/fixtrees does is it strips off the common directory path
The new_codebase would become a1.py a2.py and similarly the old_codebse would become a1.py , a4.py

The original path would be lost, we are actually comapring the deltas based upon the new paths and then we are comparing the deltas based on their sha1 values .
But in delta reports we need the original paths and the aligned path so for that reason we are creating a dict of it .

Lets just ignore/not do any alignment for this first implementation.

steven-esser · 2021-04-10T01:27:36Z

+        """ 
+            in self.new_files_fingerprint we keep the key as files original path and the value as its fingerprint value
+        """
+        self.old_files_original_path = dict()


Same here, why do we need this dict again?

The same reason as above is for keeping the old_files path and the above was for new_files.

Lets just ignore/not do any alignment for this first implementation.

steven-esser · 2021-04-10T01:28:38Z

+        try:
+            self.codebase1 = VirtualCodebase(new_path)
+            self.codebase2 = VirtualCodebase(old_path)
+
+        except Exception as exception:
+            self.errors.append(str(exception))


What exception are we catching here. I would prefer this to be specific instead of the catch-all Exception

Okay can be changed,
The exception is mainly when we do not find the files in the disk for the VirtualCodebase

Changed in commit e6a6cbd

steven-esser · 2021-04-10T01:30:16Z

+        except Exception as exception:
+            self.errors.append(str(exception))
+
+        if self.codebase1 is not None and self.codebase2 is not None:


Can we just change this to

if self.codebase1 is None or self.codebase2 is None: continue rest of logic

This is easier to read and we can lose an indentation.

Not very sure we can continue in case of if

steven-esser · 2021-04-10T01:31:00Z

-            utils.fix_trees(self.new.files, self.old.files)
+            self.new_files_original_path , self.old_files_original_path = utils.fix_trees(self.codebase1, self.codebase2)


I am not sure we need this anymore now that we are using VirtualCodebase

In fact, I think we can throw all the stuff having to do with path alignment and original paths out.

We should treat the VirtualCodebases as the are loaded and not try to manipulate paths.

@MaJuRG we must align the path as we are making delta comparison based upon it.
as I explained here https://github.com/nexB/deltacode/pull/167/files#r611008180

Virtual Codebase does not take care of the stipped of path after alignment.
Now if we are not having the aligned path whenever we change the root directory(which we supporrt currently) even the files which are moved will be difficult to trace. So we are keeping that align scans

Now if we are not planning to have the allign scans I think we should also treat files like old/a1.py and new/a1.py as added even if they are having the same sha1.

But we don't do it currently and they are treated as same.

We should treat the VirtualCodebases as the are loaded and not try to manipulate paths.

We are doing the path manipulation to compare the deltas.

steven-esser · 2021-04-10T01:34:06Z

+                                old_files_sha1_considered_in_deltas[old_resource.sha1] = 1
+                                self.stats.num_unmodified += 1
+                                new_resource.path = path_new
+                                old_resource.path = path_old
+                                delta = Delta(0, new_resource, old_resource)
+                                delta.status = "unmodified"
+                                self.deltas.append(delta)
+                                break
+                            else:
+                                old_files_sha1_considered_in_deltas[old_resource.sha1] = 1
+                                self.stats.num_modified += 1
+                                new_resource.path = path_new
+                                old_resource.path = path_old
+                                delta = Delta(20, new_resource, old_resource)
+                                delta.status = "modified"
+                                self.deltas.append(delta)
+                                break
+                        else:
+                            if new_resource.sha1 == old_resource.sha1:
+                                old_files_sha1_considered_in_deltas[old_resource.sha1] = 1
+                                ADDED = False
+                                self.stats.num_moved += 1
+                                new_resource.path = path_new
+                                old_resource.path = path_old                                
+                                delta = Delta(0, new_resource, old_resource)
+                                delta.status = "moved"
+                                self.deltas.append(delta)


Its probably cleaner to have functions to create each type of Delta object (Added, modified, etc)

That way we do not have 6+ lines of code after each conditional.

Yes it can be done, thanks
It is a just a basic model which I did to show the working of the BFS traversal of the two codebases.

Changed in commit e6a6cbd

steven-esser · 2021-04-10T01:36:14Z

+    def copyrights_to_dict(self,file):
+        """
+        Given a Copyright object, return an OrderedDict with the full
+        set of fields from the ScanCode 'copyrights' value.
+        """
+
+        copyrightC = []
+        try :
+            copyrightC = file.copyrights
+        except AttributeError:
+            # arises when the ScannedResource do not have any license attribute
+            return []
+        if len(copyrightC) == 0:
+            return []
+
+        if isinstance(copyrightC[0],dict):
+            # all the copyright are in correct format
+            all_copyrights = []
+            for i in range(len(copyrightC)):
+                # we iterate over all the copyrights
+                statements = copyrightC[i].get("statements",None)
+                holders = copyrightC[i].get("holders",None)
+                d = OrderedDict([
+                    ('statements', statements),
+                    ('holders', holders)
+                ])
+                all_copyrights.append(d)
+
+            return all_copyrights
+
+    def licenses_to_dict(self,file):
+        """
+        Given a License object, return an OrderedDict with the full
+        set of fields from the ScanCode 'license' value.
+        """
+        licenseL = []
+        try:
+            licenseL = file.licenses
+        except AttributeError:
+            # arises when the ScannedResource do not have any license attribute
+            return []
+
+        if len(licenseL) == 0:
+            return []
+        if isinstance(licenseL[0],dict):
+            # the licenses are in the correct format
+            all_licenses = []
+            for i in range(len(licenseL)):
+                # we iterate over all the licenses
+                key = licenseL[i].get("key",None)
+                score = licenseL[i].get("score",None)
+                short_key = licenseL[i].get("short_name",None)
+                category = licenseL[i].get("category",None)
+                owner = licenseL[i].get("owner",None)
+                d = OrderedDict([
+                    ('key', key),
+                    ('score', score),
+                    ('short_name', short_key),
+                    ('category', category),
+                    ('owner', owner)
+                ])
+                all_licenses.append(d)
+            return all_licenses
+
+    def file_to_dict(self,deltacode, new_file = True):
+        if new_file==False and self.old_file :
+            return OrderedDict([
+                ("path",self.old_file.path),
+                ("type",self.old_file.type),
+                ("name",self.old_file.name),
+                ("size",self.old_file.size),
+                ("sha1",self.old_file.sha1),
+                ("fingerprint",deltacode.old_files_fingerprint.get(self.old_file.path,"")),
+                ("original_path",deltacode.old_files_original_path.get(self.old_file.path, self.old_file.path)),
+                # since license itself has many sub fields so we obtain it from another utility function
+                ("licenses",self.licenses_to_dict(self.old_file)),
+                # since copyright itself has many sub fields so we obtain it from another utility function
+                ("copyrights",self.copyrights_to_dict(self.old_file))       
+            ])
+        elif new_file and self.new_file:
+            return OrderedDict([
+                ("path",self.new_file.path),
+                ("type",self.new_file.type),
+                ("name",self.new_file.name),
+                ("size",self.new_file.size),
+                ("sha1",self.new_file.sha1),
+                ("fingerprint",deltacode.new_files_fingerprint.get(self.new_file.path,"")),
+                ("original_path",deltacode.new_files_original_path.get(self.new_file.path, self.new_file.path)),
+                # since license itself has many sub fields so we obtain it from another utility function
+                ("licenses",self.licenses_to_dict(self.new_file)),
+                # since copyright itself has many sub fields so we obtain it from another utility function
+                ("copyrights",self.copyrights_to_dict(self.new_file))          
+            ])


All of this is very ugly. I think we are better off ignoring the license and copyright checks and adapt this later once VirtualCodebase is integrated into determine_delta. We can probably handle the copyright and license checking stuff much more cleanly then.

Refactore the entire block above in cde45ef , 75977c4 , and 99c153d

steven-esser · 2021-04-10T01:37:19Z

+    try:
+
+        if not delta.new_file.licenses and delta.old_file.licenses:
+            delta.update(15, 'license info removed')
+            return
+
+        new_licenses = delta.new_file.licenses or []
+        old_licenses = delta.old_file.licenses or []
+
+        new_categories = set(license.get('category','') for license in new_licenses)
+        old_categories = set(license.get('category','') for license in old_licenses)
+
+        if delta.new_file.licenses and not delta.old_file.licenses:
+            delta.update(20, 'license info added')
+
+            for category in new_categories:
+                # no license ==> 'Copyleft Limited'or higher
+                if category in unique_categories:
+                    delta.update(20, category.lower() + ' added')
+                # no license ==> 'Permissive' or 'Public Domain'
+                else:
+                    delta.update(0, category.lower() + ' added')
+            return
+
+        new_keys = set(license['key'] for license in delta.new_file.licenses)
+        old_keys = set(license['key'] for license in delta.old_file.licenses)
+
+        if new_keys != old_keys:
+
+            delta.update(10, 'license change')
+            for category in new_categories - old_categories:
+                unique_categories_in_old_file = len(old_categories & unique_categories)
+                # 'Permissive' or 'Public Domain' ==> 'Copyleft Limited' or higher
+                if unique_categories_in_old_file == 0 and category in unique_categories:
+                    delta.update(20, category.lower() + ' added')
+                # at least 1 category in the old file was 'Copyleft Limited' or higher ==> 'Copyleft Limited' or higher
+                elif unique_categories_in_old_file != 0 and category in unique_categories:
+                    delta.update(10, category.lower() + ' added')
+                # 'Permissive' or 'Public Domain' ==> 'Permissive' or 'Public Domain' if not in old_categories
+                elif category not in unique_categories:
+                    delta.update(0, category.lower() + ' added')
+    except AttributeError:


See my comment above. This whole block is really ugly. I think we are better off ignoring everything having to do with license and copyright checks for now. We can add this later in a much more clean way.

steven-esser · 2021-04-10T01:38:10Z

+def align_trees(codebase1, codebase2):
    a_names = defaultdict(list)
-    for a_file in a_files:
-        a_names[a_file.name].append(a_file)
-    a_uniques = {k: v[0] for k, v in a_names.items() if len(v) == 1}
+    for resource in codebase1.walk():
+        a_names[resource.name].append(resource)
+    a_uniques = {k : v[0] for k, v in a_names.items() if len(v) == 1}

    b_names = defaultdict(list)
-    for b_file in b_files:
-        b_names[b_file.name].append(b_file)
-    b_uniques = {k: v[0] for k, v in b_names.items() if len(v) == 1}
+    for resource in codebase2.walk():
+        b_names[resource.name].append(resource)
+    b_uniques = {k : v[0] for k, v in b_names.items() if len(v) == 1}


Like mentioned above, I think we can safely ignore all the logic around aligning trees and storing old_paths etc.

pratik0316 · 2021-06-02T17:48:28Z

@MaJuRG I have restructured the code since the last review, I will modify the test cases soon.
Please review it once

steven-esser · 2021-06-03T23:57:49Z

+        old_holders = set(holder for copyright in old_copyrights for holder in copyright['holders'])
+        if new_holders != old_holders:
+            delta.update(5, 'copyright change')
+    except Exception as exception:


What line of code in the try block raises the exceptions? and is it a single exception or more than one.

No @MaJuRG it was an exception of more than one :(

Ok we need to refactor this a bit, I do not like catching every possible exception and doing absolutely nothing. What exact exceptions are being raised here?

The error is raised when the Resource object is not having the copyright attributes

Ok, we could put a guard in before performing operations on the .copyright data to avoid having this exception raised

Yes that could be done doing it shortly

Changed in 85c85ed

steven-esser · 2021-06-03T23:58:59Z

-    equal paths for two files that are the same in 'a' and 'b'.
-    """
-    # we need to find one uniquly named file that exists in 'a' and 'b'.
+def align_trees(codebase1, codebase2):


Do we still need this function?

I thaught of removing align trees , but then our scoring system is based upon after aligning the path , so I planned to keep it.

Can you point me to the line (s) of code that uses path for scoring?

https://github.com/nexB/deltacode/blob/bf767c2a05ca12f40d77036a6f30375f9fbf1c0e/src/deltacode/__init__.py#L107

We are basically creating deltas after aligning of paths(based on the aligned path ) in old architecture and
after the creation of the delta we assign scores based on removed/added... and all the test cases/expected results are in the Design are based upon it too.
So i thaught of keeping the alignment for now

https://github.com/nexB/deltacode/blob/bf767c2a05ca12f40d77036a6f30375f9fbf1c0e/src/deltacode/__init__.py#L122

The path which we are enumerating actually refers to the aligned path

But does this work with virtual codebase?

Not directly actually as we are having the original_path in Resources , and to alling the path the path we just need two OFFSET for path aligning

steven-esser · 2021-06-05T15:06:15Z

 install_requires =
    bitarray==1.1.0
-    commoncode
+    commoncode==21.5.12


We probably want to relax this to >=

pratik0316 · 2021-06-09T08:21:39Z


        assert stats_object == expected

+    @pytest.mark.xfail(reason='Support for fingerprint required')


tagging as xfail for now due to fingerprint

pratik0316 · 2021-06-16T06:30:05Z

@MaJuRG please review it once :)

JonoYang

@Pratikrocks Thanks for the PR! I've left some comments for you. I've noticed that some places could use comments explaining the more complicated functions.

JonoYang · 2021-06-22T23:31:23Z

-from deltacode.models import File
-from deltacode.models import Scan
 from deltacode import utils
+from deltacode.exceptions import FileError as FileError


Suggested change

from deltacode.exceptions import FileError as FileError

from deltacode.exceptions import FileError

I don't think we need to import FileError as FileError?

JonoYang · 2021-06-22T23:48:51Z

+                old_resource = self.codebase2.get_resource_from_path(path_new)
+
+                if old_resource and old_resource.sha1 == new_resource.sha1:
+                    old_resource_considered[old_resource.path] = 1


old_resource_considered should be a set(). There is no need to set values for paths you want to keep track of paths you looked at.

>>> old_resource_considered = set() >>> old_resource_considered.add('test_path') >>> 'test_path' in old_resource_considered True

JonoYang · 2021-06-23T00:10:06Z

-                        self.stats.num_unmodified += 1
-                        self.deltas.append(delta)
+                ADDED = True
+                for old_resource in self.codebase2.walk():


Please add some comments explaining what the following code block does. It is not immediately obvious to whoever is looking at the code.

Sure, I will be adding it

JonoYang · 2021-06-23T00:50:55Z

+                else "{} is expected to be a file".format(old_path)
+            )
+            raise FileError(error_message)
+        self.stats = Stat(


I would recommend unpacking the value you want out of .compute_counts() and pass it into Stat() rather than passing the entire tuple returned by .compute_counts()

codebase1_file_count, _, _ = self.codebase1.compute_counts() codebase2_file_count, _, _ = self.codebase2.compute_counts() self.stats = Stat(codebase1_file_count, codebase2_file_count)

The constructor for Stat() should be changed back to the way it was before, where it expects an integer as input and not a tuple.

JonoYang · 2021-06-23T00:51:41Z

    def __init__(self, new_files_count, old_files_count):
-        self.new_files_count = new_files_count
-        self.old_files_count = old_files_count
+        self.new_files_count = new_files_count[0]


Suggested change

self.new_files_count = new_files_count[0]

self.new_files_count = new_files_count

The constructor should only accept ints, as explained above where Stat() is called.

Okay changing it

JonoYang · 2021-06-23T00:54:37Z

+#
+
+
+class FileError(Exception):


I feel like this does not need its own file and could be added to src/deltacode/__init__.py.

Agreed! removed it from there

JonoYang · 2021-06-23T01:23:41Z

    a_names = defaultdict(list)
-    for a_file in a_files:
-        a_names[a_file.name].append(a_file)
+    for resource in codebase1.walk():


Please re-add the docstring to this function and update it to describe what is going on in this function.

Added comments and doc strings at all required areas

JonoYang · 2021-06-23T01:33:41Z

-        assert 'copyright change' in test_delta.factors
+        deltas = test_delta.deltas
+
+        assert deltas[0].score == 25


It is better to address to assign the first element of deltas to a variable and use that rather than repeatedly getting the delta object from the list by index.

deltas = test_deltas.deltas # ensure that deltas is not empty assert deltas delta = deltas[0] assert len(delta.factors) == 1 assert 'copyright change' in delta.factors

Please update this in other places where this occurs.

Yes that's better changing it soon !

pratik0316 · 2021-06-27T13:12:16Z

+
+    NEW_CODEBASE_OFFSET = 0
+    OLD_CODEBASE_OFFSET = 0
+


@MaJuRG basically these offset would handle the path alignment things..

Signed-off-by: Pratik Dey <pratikrocks.dey11@gmail.com>

pratik0316 · 2021-07-15T14:46:47Z

all green

pratik0316 added 2 commits March 23, 2021 19:35

Test

e2026ef

Signed-off-by: Pratik Dey <pratikrocks.dey11@gmail.com>

resolved dependency

477cc58

Signed-off-by: Pratik Dey <pratikrocks.dey11@gmail.com>

pratik0316 commented Apr 4, 2021

View reviewed changes

steven-esser suggested changes Apr 10, 2021

View reviewed changes

pratik0316 force-pushed the VC_determine_delta branch 8 times, most recently from 91b7482 to 01f40e7 Compare May 30, 2021 13:43

pratik0316 requested a review from steven-esser June 2, 2021 17:48

steven-esser reviewed Jun 3, 2021

View reviewed changes

steven-esser reviewed Jun 5, 2021

View reviewed changes

pratik0316 commented Jun 9, 2021

View reviewed changes

pratik0316 force-pushed the VC_determine_delta branch 4 times, most recently from e288619 to 25d9e5d Compare June 11, 2021 19:08

pratik0316 closed this Jun 16, 2021

pratik0316 reopened this Jun 16, 2021

pratik0316 assigned pratik0316 and unassigned pratik0316 Jun 16, 2021

pratik0316 requested a review from steven-esser June 16, 2021 06:30

pratik0316 force-pushed the VC_determine_delta branch 2 times, most recently from 778b5ef to 8b02c5a Compare June 20, 2021 09:30

pratik0316 requested review from JonoYang and pombredanne June 20, 2021 09:32

JonoYang requested changes Jun 23, 2021

View reviewed changes

pratik0316 linked an issue Jun 24, 2021 that may be closed by this pull request

Migrate to using VirtualCodebase from the latest scancode #101

Closed

pratik0316 added the GSOC Project/issues to be done during the GSOC Time frame label Jun 24, 2021

pratik0316 commented Jun 27, 2021

View reviewed changes

pratik0316 force-pushed the VC_determine_delta branch 7 times, most recently from 70f82ad to 7a0a5e2 Compare July 9, 2021 13:15

base outline

ee639bf

Signed-off-by: Pratik Dey <pratikrocks.dey11@gmail.com>

pratik0316 force-pushed the VC_determine_delta branch from 7a0a5e2 to ee639bf Compare July 9, 2021 13:16

Merge branch 'develop' into VC_determine_delta

92c3355

steven-esser approved these changes Jul 15, 2021

View reviewed changes

steven-esser merged commit a095146 into aboutcode-org:develop Jul 15, 2021

		utils.fix_trees(self.new.files, self.old.files)
		self.new_files_original_path , self.old_files_original_path = utils.fix_trees(self.codebase1, self.codebase2)


		assert stats_object == expected

		@pytest.mark.xfail(reason='Support for fingerprint required')

	from deltacode.exceptions import FileError as FileError
	from deltacode.exceptions import FileError

	self.new_files_count = new_files_count[0]
	self.new_files_count = new_files_count

Uh oh!

Uh oh!

Conversation

pratik0316 commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pratik0316 Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratik0316 commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratik0316 Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratik0316 Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratik0316 Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratik0316 commented Jun 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steven-esser Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratik0316 commented Apr 4, 2021 •

edited

Loading

pratik0316 Apr 4, 2021 •

edited

Loading

pratik0316 commented Apr 4, 2021 •

edited

Loading

pratik0316 Apr 10, 2021 •

edited

Loading

pratik0316 Apr 10, 2021 •

edited

Loading

pratik0316 Apr 17, 2021 •

edited

Loading

steven-esser Jun 5, 2021 •

edited

Loading

pratik0316 Jun 27, 2021 •

edited

Loading