PRO-937: Add `create` and `insert` table endpoints to sdk #114

waddahAldrobi · 2024-03-15T15:58:33Z

In this PR, we add support for the new table endpoints in the SDK:

We move the upload_csv function with the new endpoints into api/table.py to consolidate all table endpoints in one file

…reate/ endpoint

dsalv · 2024-03-18T12:11:16Z

dune_client/api/extensions.py

@@ -316,40 +317,6 @@ def download_csv(
            ),
        )

-    ############################


I think some people might still be depending on this. We might need to keep it for backwards compatibility

It was moved here
https://github.com/duneanalytics/dune-client/pull/114/files/dd6d30d78502ea752bd6c04cc1ea7299da2e1347#diff-480e443b4b14d9d3ff5327a749050ff3e17aa60adbdd304fa90b524d89644073R19

Experience for users would not change

dune_client/api/table.py

vegarsti

Looks good! A few things I'd want changed

dune_client/api/table.py

hawkaa

Fantastic!

I think we should wait by merging this until we've done PRO-1039 and PRO-1040. We are not in a hurry to get this out asap.
In my opinion, using path as input rather than bytes is the wrong abstraction for the lowest level endpoints. The users of the library will have to read the file and provide the correct extension. We do this for upload_csv.

dune_client/api/base.py

dune_client/api/extensions.py

dune_client/api/table.py

helanto

Some minor questions.

dune_client/api/base.py

tests/e2e/test_client.py

helanto · 2024-03-19T08:51:33Z

dune_client/api/table.py

+        schema: List[Dict[str, str]],
+        description: str = "",
+        is_private: bool = False,
+    ) -> Any:


Why Any ?

That is what self._post returns.
This was already implemented, but we can change that once we have something returned

If so, should this change now? 😄

I am trying to say that Any is not a useful return type. What should users expect back when calling the create_table or insert_table functions ?

but we can change that once we have something returned

But we should already have a response returned from the API, right ?

That is what self._post returns.

The fact that self._post returns Any, might have been a bad design / tech debt. If this is part of the public API, which we cannot change, maybe we can consider returning a more useful type here.

If not an easy change, let's keep it like that.

I agree that it was probably not the best decision.
This function is kind of unnecessary, which is used to handle _post, _get, and _patch responses

dune-client/dune_client/api/base.py

Lines 139 to 150 in e3f6c98

def _handle_response(self, response: Response) -> Any:

"""Generic response handler utilized by all Dune API routes"""

try:

# Some responses can be decoded and converted to DuneErrors

response_json = response.json()

self.logger.debug(f"received response {response_json}")

return response_json

except JSONDecodeError as err:

# Others can't. Only raise HTTP error for not decodable errors

response.raise_for_status()

raise ValueError("Unreachable since previous line raises") from err

Should have just returned the Response object.
response.json() returns Any, so this will take a bit of work.

And as such, I don't think is worth it.

This was already implemented, but we can change that once we have something returned

I take this back, this was not as simple as I thought it would be.

Thanks 🖖

Sorry folks, this was my fault ☹️ . But these internal private methods return Any and the idea is to convert them to the expected type inside these pubic facing interfaces. So, for example... we have these Object.from_dict() methods that convert Any responses to the expected types:

dune-client/dune_client/api/execution.py

Lines 66 to 72 in b9db190

def get_execution_status(self, job_id: str) -> ExecutionStatusResponse:

"""GET status from Dune API for `job_id` (aka `execution_id`)"""

response_json = self._get(route=f"/execution/{job_id}/status")

try:

return ExecutionStatusResponse.from_dict(response_json)

except KeyError as err:

raise DuneError(response_json, "ExecutionStatusResponse", err) from err

Here would have been the same thing. Since I am currently implementing these API routes in the Node project, I will submit a corresponding PR for this one.

Here is the related PR: #117

tests/e2e/test_client.py

vegarsti · 2024-03-19T10:57:52Z

dune_client/api/base.py

+        headers: Dict[str, str] = {},
+    ) -> Any:
+        # pylint: disable=dangerous-default-value


Mutable default arguments is indeed dangerous in Python. So please remove the pylint thing and change: This will only be initialized once, and any changes to it will be durable. See https://docs.python-guide.org/writing/gotchas/

So we need to change it to Optional[Dict[str, str]] = None and then handle it in the body of the method

This was done in response to this.
#114 (comment)

But I can change it back

vegarsti · 2024-03-19T10:59:16Z

dune_client/api/table.py

+        schema: List[Dict[str, str]],
+        description: str = "",
+        is_private: bool = False,
+    ) -> Any:


If so, should this change now? 😄

dune_client/api/table.py

vegarsti · 2024-03-19T11:00:17Z

dune_client/api/table.py

+        self,
+        namespace: str,
+        table_name: str,
+        schema: List[Dict[str, str]],


At some point we probably want to create an enum of the possible types, but not yet!

Do you happen to have a list of all the possible types?

Yup they're defined here
https://dune-tables-docs.mintlify.app/api-reference/tables/endpoint/create

(Link is in docstring)

vegarsti

Great!

Sorry dismissing to unblock PR merge

bh2smith · 2024-03-25T09:31:54Z

dune_client/api/table.py

+class TableAPI(BaseRouter):
+    """
+    Implementation of Table endpoints - Plus subscription only
+    https://docs.dune.com/api-reference/tables/


Note: There is no landing page for the Tables API. This url redirects to https://docs.dune.com/api-reference/tables/endpoint/create and there only appears to be one direct link in the docs page (inside a comment on uploadCSV)

Hmm, you are correct.
Opening a ticket internally

Great callout! It should probably be an "Overview" or similar like with the other endpoint groups.

bh2smith · 2024-03-25T11:32:45Z

Hey @waddahAldrobi I am loving all these great contributions, but do you think you could "Squash Merge" your PRs so that the commit history on main is easier to navigate?

This is difficult to follow and contains a lot of irrelevant stuff.

waddahAldrobi · 2024-03-25T11:36:13Z

but do you think you could "Squash Merge" your PRs so that the commit history on main is easier to navigate?

@bh2smith 💯 that would have been a much better idea 👍

waddahAldrobi added 4 commits March 15, 2024 11:57

Black linting

3853295

Seperate table endpoints into TableAPI class and support for /table/c…

f97dc83

…reate/ endpoint

Added support for table/insert endpoint

fd95ef8

Fix test_create_table_success retrun type

dd6d30d

waddahAldrobi changed the title ~~[WIP] Pro 937 add table endpoints to sdk~~ Pro 937 add table endpoints to sdk Mar 18, 2024

waddahAldrobi requested review from a team and hawkaa March 18, 2024 11:46

waddahAldrobi changed the title ~~Pro 937 add table endpoints to sdk~~ PRO-937: Add create and insert table endpoints to sdk Mar 18, 2024

waddahAldrobi marked this pull request as ready for review March 18, 2024 11:46

dsalv reviewed Mar 18, 2024

View reviewed changes

dune_client/api/table.py Outdated Show resolved Hide resolved

vegarsti reviewed Mar 18, 2024

View reviewed changes

hawkaa previously requested changes Mar 18, 2024

View reviewed changes

waddahAldrobi added 5 commits March 18, 2024 09:28

Refactored insert_table to use data bytes into of path

6a3a1c5

Remove unnecessary try-except

2f14b9a

Default headers to {}

3338e36

Add support for creating private tables

eaa0fdb

Remove must be public comment limitation

8c7f8e6

helanto reviewed Mar 19, 2024

View reviewed changes

waddahAldrobi added 3 commits March 19, 2024 05:29

Changing create endpoint to use params for namespace and table_name

6130465

Resolve linting issues

d5fe607

Update create_table rest with latest response

e3f6c98

waddahAldrobi requested review from hawkaa, vegarsti, helanto and dsalv March 19, 2024 10:51

vegarsti reviewed Mar 19, 2024

View reviewed changes

waddahAldrobi added 3 commits March 19, 2024 07:22

Fixed a typo

0c1c14d

Fix linting

7fdbfa5

Skip create_table_success unit test

4e11311

Change headers in post back to default to None

642cc49

waddahAldrobi requested a review from vegarsti March 19, 2024 12:13

vegarsti approved these changes Mar 19, 2024

View reviewed changes

waddahAldrobi merged commit 1cbe59c into main Mar 19, 2024
3 checks passed

bh2smith reviewed Mar 25, 2024

View reviewed changes

bh2smith deleted the PRO-937-add-table-endpoints-to-sdk branch March 25, 2024 09:51

bh2smith mentioned this pull request Mar 25, 2024

Return Types on Create and Insert Table #117

Merged

waddahAldrobi mentioned this pull request Apr 2, 2024

Add delete table endpoint to sdk #119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRO-937: Add `create` and `insert` table endpoints to sdk #114

PRO-937: Add `create` and `insert` table endpoints to sdk #114

waddahAldrobi commented Mar 15, 2024 •

edited

Loading

dsalv Mar 18, 2024

waddahAldrobi Mar 18, 2024 •

edited

Loading

vegarsti left a comment

hawkaa left a comment

helanto left a comment

helanto Mar 19, 2024

waddahAldrobi Mar 19, 2024

vegarsti Mar 19, 2024

helanto Mar 19, 2024

waddahAldrobi Mar 19, 2024

waddahAldrobi Mar 19, 2024

helanto Mar 19, 2024

bh2smith Mar 25, 2024 •

edited

Loading

bh2smith Mar 25, 2024

vegarsti Mar 19, 2024 •

edited

Loading

waddahAldrobi Mar 19, 2024

vegarsti Mar 19, 2024

waddahAldrobi Mar 19, 2024

vegarsti Mar 19, 2024

vegarsti Mar 19, 2024

bh2smith Mar 25, 2024

waddahAldrobi Mar 25, 2024

vegarsti left a comment

bh2smith Mar 25, 2024

waddahAldrobi Mar 25, 2024

hawkaa Mar 25, 2024

bh2smith commented Mar 25, 2024 •

edited

Loading

waddahAldrobi commented Mar 25, 2024

	def _handle_response(self, response: Response) -> Any:
	"""Generic response handler utilized by all Dune API routes"""
	try:
	# Some responses can be decoded and converted to DuneErrors
	response_json = response.json()
	self.logger.debug(f"received response {response_json}")
	return response_json
	except JSONDecodeError as err:
	# Others can't. Only raise HTTP error for not decodable errors
	response.raise_for_status()
	raise ValueError("Unreachable since previous line raises") from err

	def get_execution_status(self, job_id: str) -> ExecutionStatusResponse:
	"""GET status from Dune API for `job_id` (aka `execution_id`)"""
	response_json = self._get(route=f"/execution/{job_id}/status")
	try:
	return ExecutionStatusResponse.from_dict(response_json)
	except KeyError as err:
	raise DuneError(response_json, "ExecutionStatusResponse", err) from err

PRO-937: Add create and insert table endpoints to sdk #114

PRO-937: Add create and insert table endpoints to sdk #114

Conversation

waddahAldrobi commented Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

waddahAldrobi Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

vegarsti left a comment

Choose a reason for hiding this comment

hawkaa left a comment

Choose a reason for hiding this comment

helanto 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bh2smith Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegarsti Mar 19, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegarsti 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

bh2smith commented Mar 25, 2024 • edited Loading

waddahAldrobi commented Mar 25, 2024

PRO-937: Add `create` and `insert` table endpoints to sdk #114

PRO-937: Add `create` and `insert` table endpoints to sdk #114

waddahAldrobi commented Mar 15, 2024 •

edited

Loading

waddahAldrobi Mar 18, 2024 •

edited

Loading

bh2smith Mar 25, 2024 •

edited

Loading

vegarsti Mar 19, 2024 •

edited

Loading

bh2smith commented Mar 25, 2024 •

edited

Loading