[AL-2038] Faster transforms #2094

FayazRahman · 2023-01-05T08:22:06Z

🚀 🚀 Pull Request

Checklist:

My code follows the style guidelines of this project and the Contributing document
I have commented my code, particularly in hard-to-understand areas
I have kept the coverage-rate up
I have performed a self-review of my own code and resolved any problems
I have checked to ensure there aren't any other open Pull Requests for the same change
I have described and made corresponding changes to the relevant documentation
New and existing unit tests pass locally with my changes

Changes

…fy_new_transform

codecov · 2023-01-06T14:58:06Z

Codecov Report

Patch coverage: 96.15% and project coverage change: +0.34 🎉

Comparison is base (1536f94) 88.15% compared to head (3be8d22) 88.49%.

❗ Current head 3be8d22 differs from pull request most recent head 357d0dd. Consider uploading reports for the commit 357d0dd to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2094      +/-   ##
==========================================
+ Coverage   88.15%   88.49%   +0.34%     
==========================================
  Files         291      291              
  Lines       32632    32853     +221     
==========================================
+ Hits        28767    29074     +307     
+ Misses       3865     3779      -86

Flag	Coverage Δ
unittests	`88.49% <96.15%> (+0.34%)`	⬆️

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

Impacted Files	Coverage Δ
deeplake/core/polygon.py	`89.10% <66.66%> (-0.37%)`	⬇️
deeplake/core/transform/transform_tensor.py	`91.95% <90.90%> (+13.04%)`	⬆️
deeplake/util/transform.py	`94.65% <97.10%> (+1.36%)`	⬆️
deeplake/core/chunk_engine.py	`94.27% <100.00%> (+0.59%)`	⬆️
deeplake/core/linked_chunk_engine.py	`91.70% <100.00%> (ø)`
deeplake/core/tensor_link.py	`91.15% <100.00%> (+4.35%)`	⬆️
deeplake/core/transform/test_transform.py	`99.36% <100.00%> (+0.04%)`	⬆️
deeplake/core/transform/transform.py	`98.52% <100.00%> (-0.02%)`	⬇️
deeplake/core/transform/transform_dataset.py	`98.64% <100.00%> (+5.31%)`	⬆️
deeplake/util/class_label.py	`98.83% <100.00%> (+0.01%)`	⬆️
... and 1 more

... and 128 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…fy_new_transform

deeplake/core/transform/transform_dataset.py

…fy_new_transform

deeplake/core/transform/transform_dataset.py

adolkhan · 2023-04-12T11:23:37Z

deeplake/util/transform.py

+    pg_callback,
+    ignore_errors,
+):
+    n = len(data_slice)


Either create several methods or a class. One solution could be to initialize an object replace each if statement with a method. To be honest I prefer more creating object for this method.

Nested try catch block seems complicated, can you put some comments and elaborate more on why we need a nested try catch?

this function only calls other functions and does almost nothing else.

I can't agree with it, you are doing bunch of checks and try catches, it is too much for a single function

Also pls add comments to try catch blocks, so that we will understand what it tries to catch

The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

there are very few checks and try catch blocks are there for a reason. This functionn also serves only one person. please try to elaborate more what you mean.

adolkhan · 2023-04-12T11:57:38Z

deeplake/core/transform/transform_tensor.py

-        elif isinstance(value, Sample):
-            value = value.array
-        return value
+        values: List[Any] = []


a bit complicated logic, can we separate it?

For instance we can keep for loop, create a dict:

INSTANCE_TYPE_TO_APPEND_METHOD = { Sample: self.append_sample, LinkedSample: self.append_tensor_or_partial_sample, Tensor: self.append_tensor_or_partial_sample, type(None): self.append_tensor_or_partial_sample, PartialSample: self.append_tensor_or_partial_sample, LinkedTiledSample: self.append_tensor_or_partial_sample, }

put this right under TransformTesnor Class before init,

and define two methods:

def self.append_sample(self, values, item): values.append(item.array) def self.append_sample(self, values, item): values.append(np.asarray(item)) def append_item(self, values, item): values.append(item)

And finally this code block will look as follows:

for item in items: append_method = self.INSTANCE_TYPE_TO_APPEND_METHOD[type(item)] append_method(values, item) if squeeze: values = values[0] return values

This way we will make our code more isolated and easy to understand

The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T12:00:20Z

deeplake/core/transform/transform_tensor.py

-            slice_list=new_slice_list,
-        )
+        idx = self.idx
+        if self.numpy_only:


Same here triple nested if statements are complicated, can we separate it?

We can create several methods. The goal is to make it more readable, and more explicit on what happens if some if statements are not satisfied.

The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T12:02:31Z

deeplake/core/transform/transform_tensor.py

-                                incoming_shape, self._ndim
-                            )
-                return self.items.append(items)
+        if self.numpy_only:


we can create a move the logic inside of the if statement to a new method. Also try to make if else statements more readable. Quite hard to grasp what is going on here.

The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T12:03:54Z

deeplake/util/transform.py

+
+
+def write_sample_to_transform_dataset(out, transform_dataset):
+    if not is_empty_transform_dataset(out):


Complicated logic for a single function. Either create several methods or a class where you will isolate all if else statements and for loops

this function is only 10 lines of code

there are 2 for loops, we need to isolate it... and if else statements

The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

This function serves a single purpose which is to write a sample to a transform dataset. Unnecessarily separating a 10-line function for the sake of doing a for loop in another function only makes it harder to follow.

adolkhan · 2023-04-12T12:04:51Z

deeplake/util/transform.py

        )
+        try:


can we create a separate method for try catch block?

No, there are only 3 lines of code in the try block

The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

FayazRahman · 2023-04-12T14:08:05Z

deeplake/util/transform.py

+        try:
+            transform_dataset.set_start_input_idx(i)
+
+            try:


this try will catch error while a sample is being transformed. it is easier to handle compared to an error after a sample has been sent to the chunk engine.

FayazRahman · 2023-04-12T14:09:03Z

deeplake/util/transform.py

+            try:
+                out = transform_sample(sample, pipeline, tensors)
+            except SampleAppendError as e:
+                if ignore_errors:


we will only ignore SampleAppendError if ignore_errors is True

FayazRahman · 2023-04-12T14:09:34Z

deeplake/util/transform.py

+        if pd and isinstance(data_slice, pd.DataFrame)
+        else data_slice
+    ):
+        try:


This try will catch errors from chunk engine

adolkhan · 2023-04-12T16:13:47Z

deeplake/util/transform.py

+    pg_callback,
+    ignore_errors,
+):
+    n = len(data_slice)


I can't agree with it, you are doing bunch of checks and try catches, it is too much for a single function

adolkhan · 2023-04-12T16:14:39Z

deeplake/util/transform.py

+    pg_callback,
+    ignore_errors,
+):
+    n = len(data_slice)


Also pls add comments to try catch blocks, so that we will understand what it tries to catch

adolkhan · 2023-04-12T16:16:52Z

deeplake/util/transform.py

        )
+        try:


The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T16:19:07Z

deeplake/util/transform.py

+
+
+def write_sample_to_transform_dataset(out, transform_dataset):
+    if not is_empty_transform_dataset(out):


there are 2 for loops, we need to isolate it... and if else statements

adolkhan · 2023-04-12T16:20:33Z

deeplake/core/transform/transform_tensor.py

-                                incoming_shape, self._ndim
-                            )
-                return self.items.append(items)
+        if self.numpy_only:


The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T16:20:48Z

deeplake/util/transform.py

+
+
+def write_sample_to_transform_dataset(out, transform_dataset):
+    if not is_empty_transform_dataset(out):


The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T16:21:41Z

deeplake/util/transform.py

+    pg_callback,
+    ignore_errors,
+):
+    n = len(data_slice)


The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T16:22:13Z

deeplake/core/transform/transform_tensor.py

-            slice_list=new_slice_list,
-        )
+        idx = self.idx
+        if self.numpy_only:


The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

adolkhan · 2023-04-12T16:22:28Z

deeplake/core/transform/transform_tensor.py

-        elif isinstance(value, Sample):
-            value = value.array
-        return value
+        values: List[Any] = []


The idea is that, we need to isolate as much as possible, single method should either do try catch or do for loop with other function calls. It is called single responsibility principle. You can take a look at it, if use this principle our code base will be much cleaner, isolated and scalable.

deeplake/core/transform/transform_dataset.py

FayazRahman added 13 commits December 28, 2022 19:21

init

1d0fe78

Merge branch 'main' of https://github.com/activeloopai/deeplake into …

2e82a74

…fy_new_transform

transforms

7257b66

Merge branch 'main' of https://github.com/activeloopai/deeplake into …

c96e488

…fy_new_transform

numpy_only

5147017

groups

1bd6dcb

transform tests passing

7f50e78

cache_size opt + fix

c988ad1

Merge branch 'main' of https://github.com/activeloopai/deeplake into …

91ed446

…fy_new_transform

label fix

0f8a8cd

pgbar

c157c3b

fix

23df780

lint

fbe14a3

FayazRahman marked this pull request as ready for review January 5, 2023 17:00

FayazRahman added 10 commits January 15, 2023 15:01

Merge branch 'main' of https://github.com/activeloopai/deeplake into …

6a6c263

…fy_new_transform

pg fix

9e6d2ab

pgbar update

a8ac859

merge confl

69dfef3

pg update

cb0a863

serial label sync

9ddef82

fix

937f34b

Merge branch 'main' of https://github.com/activeloopai/deeplake into …

03837fb

…fy_new_transform

smol fix

6b30639

Merge branch 'main' of https://github.com/activeloopai/deeplake into …

c414eab

…fy_new_transform

tatevikh requested review from farizrahman4u, AbhinavTuli, adolkhan and levongh January 20, 2023 17:48

merge confl

0a4a5e5

FayazRahman added 6 commits February 15, 2023 23:14

update

1a2f9e1

fix

cfa5667

add test

169af14

more tests

2abbdf1

fix

b4609d1

remove unused

d4752d4

AbhinavTuli reviewed Feb 20, 2023

View reviewed changes

deeplake/core/transform/transform_dataset.py Outdated Show resolved Hide resolved

AbhinavTuli reviewed Feb 20, 2023

View reviewed changes

deeplake/core/transform/transform_dataset.py Show resolved Hide resolved

AbhinavTuli approved these changes Feb 20, 2023

View reviewed changes

FayazRahman added 7 commits February 21, 2023 00:27

merge main + refactor

3bbdbbe

fix

3be8d22

updates

18a8e6d

black

f335744

Merge branch 'main' of https://github.com/activeloopai/deeplake into …

307a905

…fy_new_transform

mypy

4645592

black

6c7d0c7

tatevikh requested review from farizrahman4u and AbhinavTuli April 10, 2023 12:57

farizrahman4u approved these changes Apr 12, 2023

View reviewed changes

adolkhan suggested changes Apr 12, 2023

View reviewed changes

FayazRahman commented Apr 12, 2023

View reviewed changes

adolkhan suggested changes Apr 12, 2023

View reviewed changes

FayazRahman added 2 commits April 12, 2023 22:38

refactor

83be600

black

357d0dd

adolkhan self-requested a review April 18, 2023 07:45

adolkhan approved these changes Apr 18, 2023

View reviewed changes

FayazRahman merged commit eebde70 into main Apr 20, 2023

FayazRahman deleted the fy_new_transform branch April 20, 2023 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AL-2038] Faster transforms #2094

[AL-2038] Faster transforms #2094

FayazRahman commented Jan 5, 2023

codecov bot commented Jan 6, 2023 •

edited

Loading

adolkhan Apr 12, 2023

FayazRahman Apr 12, 2023

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

FayazRahman Apr 13, 2023

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023

FayazRahman Apr 12, 2023

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

FayazRahman Apr 13, 2023

adolkhan Apr 12, 2023

FayazRahman Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

FayazRahman Apr 12, 2023

FayazRahman Apr 12, 2023

FayazRahman Apr 12, 2023

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading



		def write_sample_to_transform_dataset(out, transform_dataset):
		if not is_empty_transform_dataset(out):

[AL-2038] Faster transforms #2094

[AL-2038] Faster transforms #2094

Conversation

FayazRahman commented Jan 5, 2023

🚀 🚀 Pull Request

Checklist:

Changes

codecov bot commented Jan 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

adolkhan Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 6, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading

adolkhan Apr 12, 2023 •

edited

Loading