feat: Allow users to configure the internal thread pool (#11) #54

alhridoy · 2023-10-08T04:20:40Z

This PR introduces a new feature that allows users to configure the internal thread pool used in the _raw_indexing_method function of the EarthEngineBackendArray class.

A new optional parameter executor_kwargs has been added to the function. This parameter is a dictionary that can be used to pass configuration options to the ThreadPoolExecutor. If executor_kwargs is not provided, an empty dictionary is used, preserving the current behavior.

This feature provides users with more flexibility and control over the execution of the function, especially in scenarios where specific thread pool configurations are needed.

Changes:

Added executor_kwargs parameter to _raw_indexing_method function in xee/ext.py.

Please review and let me know if any changes are required.

google-cla · 2023-10-08T04:20:44Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

alxmrs

Thanks for your contribution, @alhridoy! I have some initial feedback.

A couple more high-level notes:

Make sure that users can set this from open_dataset(). To accomplish this, the new keyword arg needs to be accepted in EarthEngineBackendEntrypoint().
Please update our integration tests to prove that everything is working.

alxmrs · 2023-10-08T16:44:31Z

configurable-thread-pool

@@ -0,0 +1 @@
+Branch 'configurable-thread-pool' set up to track remote branch 'main' from 'origin'.


Please remove this file.

alxmrs · 2023-10-08T16:45:09Z

xee/ext.py

@@ -634,7 +634,7 @@ def reduce_bands(x, acc):
    return target_image

  def _raw_indexing_method(
-      self, key: tuple[Union[int, slice], ...]
+      self, key: tuple[Union[int, slice], ...], executor_kwargs: Optional[dict] = None


This extra argument is named well, but I think it should be set in the constructor.

alxmrs · 2023-10-08T16:45:25Z

xee/ext.py

+    if executor_kwargs is None:
+      executor_kwargs = {}


Let's do this bit of logic in the constructor, too.

alxmrs · 2023-10-08T16:45:58Z

xee/ext.py

+     # If executor_kwargs is None, use an empty dictionary
+    if executor_kwargs is None:
+      executor_kwargs = {}
+    # Pass executor_kwargs to ThreadPoolExecutor


Comment is a bit too verbose for my taste.

alhridoy · 2023-10-12T05:09:17Z

Hi @alxmrs when i run the test file, this error pops up.

Could not import runpy module Traceback (most recent call last): File "<frozen runpy>", line 15, in <module> File "<frozen importlib.util>", line 16, in <module> File "/Users/hridoy/Desktop/open_source/Xee/xee/types.py", line 16, in <module> from typing import Union, TypedDict File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/typing.py", line 27, in <module> import contextlib File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/contextlib.py", line 7, in <module> from functools import wraps File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/functools.py", line 22, in <module> from types import GenericAlias ImportError: cannot import name 'GenericAlias' from partially initialized module 'types' (most likely due to a circular import) (/Users/hridoy/Desktop/open_source/Xee/xee/types.py)
I renamed the types.py to type.py then I run the test again, it runs but no output is shown in the terminal. any guidance to run the ext_integration_test.py file?

alhridoy · 2023-10-12T05:39:36Z

@alxmrs Thanks for your guidance. Would you check if these changes are working properly? if not, Please let me necessary the changes I need to make. Thanks

alxmrs · 2023-10-12T18:23:14Z

xee/ext.py

@@ -89,6 +89,12 @@ class EarthEngineStore(common.AbstractDataStore):

  DEFAULT_MASK_VALUE = np.iinfo(np.int32).max

+  def __init__(self, executor_kwargs=None):


There already is a constructor.

alxmrs · 2023-10-12T18:23:27Z

xee/ext_integration_test.py

@@ -358,6 +358,18 @@ def test_data_sanity_check(self):
    self.assertNotEqual(temperature_2m.min(), 0.0)
    self.assertNotEqual(temperature_2m.max(), 0.0)

+  def test_open_dataset_with_executor_kwargs(self):


Thanks for the test!

alxmrs · 2023-10-12T18:23:42Z

Please fix the merge conflicts in this branch.

alhridoy · 2023-10-12T21:37:43Z

Hi @alxmrs . Thanks for the feedback. ,I added a new parameter executor_kwargs has been added to the init method also resolved merge conflict.

alxmrs

Definitely looking better. Here are a few more additional suggestions.

alxmrs · 2023-10-13T19:37:38Z

xee/ext.py

+    # Initialize executor_kwargs
+    if executor_kwargs is None:
+      self.executor_kwargs = {}
+    else:
+      self.executor_kwargs = executor_kwargs


Suggested change

# Initialize executor_kwargs

if executor_kwargs is None:

self.executor_kwargs = {}

else:

self.executor_kwargs = executor_kwargs

if executor_kwargs is None:

executor_kwargs = {}

self.executor_kwargs = executor_kwargs

Please initialize this at the top or bottom of the __init__.

alxmrs · 2023-10-13T19:37:56Z

xee/ext.py

@@ -706,7 +714,7 @@ def reduce_bands(x, acc):
    return target_image

  def _raw_indexing_method(
-      self, key: tuple[Union[int, slice], ...]
+      self, key: tuple[Union[int, slice], ...] 


Please remove extra space (to minimize the diff).

alxmrs · 2023-10-13T19:39:24Z

xee/ext.py

@@ -845,6 +854,7 @@ def open_dataset(
      primary_dim_name: Optional[str] = None,
      primary_dim_property: Optional[str] = None,
      ee_mask_value: Optional[float] = None,
+      executor_kwargs: Optional[dict] = None,


In addition to adding it to the arguments, please update the docstring.

…methods, updates docstring

…g_method and updated docstrings

alhridoy · 2023-10-15T22:53:27Z

Hi @alxmrs , Would you check the new commits address your comments? If there is still any issue I would be happy to work on that.

alxmrs · 2023-10-15T23:08:54Z

It feels like you are using a bot to make this patch. I'll be happy to work with you to add this feature. First, please do a self review before I make any additional reviews.

alhridoy · 2023-11-09T00:32:49Z

Thanks! Please give me couple of weeks.

feat: Allow users to configure the internal thread pool (google#11)

7de6eb7

alxmrs requested changes Oct 8, 2023

View reviewed changes

Al-Iqram Elahee added 2 commits October 11, 2023 23:34

Moved executor_kwargs logic to constructor and removed unnecessary file

2d464f2

Remove unnecessary configurable-thread-pool file

d445578

alxmrs reviewed Oct 12, 2023

View reviewed changes

alhridoy added 2 commits October 12, 2023 15:30

Added executor_kwargs to EarthEngineStore

4231270

Merge branch 'main' into configurable-thread-pool

e643352

alxmrs reviewed Oct 13, 2023

View reviewed changes

alhridoy added 2 commits October 15, 2023 02:31

Added executor_kwargs parameter to EarthEngineStore and open_dataset …

4cd311d

…methods, updates docstring

Added executor_kwargs parameter, removed extra space from raw_indexin…

ec80d19

…g_method and updated docstrings

alxmrs closed this Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow users to configure the internal thread pool (#11) #54

feat: Allow users to configure the internal thread pool (#11) #54

alhridoy commented Oct 8, 2023

google-cla bot commented Oct 8, 2023

alxmrs left a comment

alxmrs Oct 8, 2023

alxmrs Oct 8, 2023

alxmrs Oct 8, 2023

alxmrs Oct 8, 2023

alhridoy commented Oct 12, 2023

alhridoy commented Oct 12, 2023

alxmrs Oct 12, 2023

alxmrs Oct 12, 2023

alxmrs commented Oct 12, 2023

alhridoy commented Oct 12, 2023

alxmrs left a comment

alxmrs Oct 13, 2023

alxmrs Oct 13, 2023

alxmrs Oct 13, 2023

alxmrs Oct 13, 2023

alhridoy commented Oct 15, 2023

alxmrs commented Oct 15, 2023

alhridoy commented Nov 9, 2023

		@@ -0,0 +1 @@
		Branch 'configurable-thread-pool' set up to track remote branch 'main' from 'origin'.

		@@ -89,6 +89,12 @@ class EarthEngineStore(common.AbstractDataStore):

		DEFAULT_MASK_VALUE = np.iinfo(np.int32).max

		def __init__(self, executor_kwargs=None):

feat: Allow users to configure the internal thread pool (#11) #54

feat: Allow users to configure the internal thread pool (#11) #54

Conversation

alhridoy commented Oct 8, 2023

google-cla bot commented Oct 8, 2023

alxmrs left a 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

alhridoy commented Oct 12, 2023

alhridoy commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxmrs commented Oct 12, 2023

alhridoy commented Oct 12, 2023

alxmrs left a 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

alhridoy commented Oct 15, 2023

alxmrs commented Oct 15, 2023

alhridoy commented Nov 9, 2023