Skip to content

Conversation

mnoszczak
Copy link
Contributor

@mnoszczak mnoszczak commented Mar 19, 2023

AL-5278
AL-5279
AL-5280

Edit: Added test cases for all the methods, no need to run the below

Need to prepare staging acc to add tests, right now can be tested by checking out to the branch and:

pip install -e .  
from labelbox import Client
API_KEY = API_KEY
ENDPOINT = STAGE_URL
SLICE_ID = 

client = Client(api_key=API_KEY, endpoint=ENDPOINT)
slice = client.get_catalog_slice(SLICE_ID)
task = slice.export_v2(params={"performance_details": False, "label_details": True})

task.wait_till_done()
task.result

@mnoszczak mnoszczak force-pushed the mno/al-5278 branch 2 times, most recently from 7abc5c9 to f6c071b Compare March 19, 2023 02:44
@mnoszczak mnoszczak changed the title [AL-5278] Add catalog slice SDK support [AL-5278] [AL-5279] [AL-5260] Add catalog slice, data row, dataset support Mar 19, 2023
@mnoszczak mnoszczak force-pushed the mno/al-5278 branch 4 times, most recently from 0773af8 to 4cfaf25 Compare March 19, 2023 22:08
@mnoszczak mnoszczak requested a review from kkim-labelbox March 20, 2023 11:43
"""
query_str = """
query getSavedQueryPyApi($id: ID!) {
query_str = """query getSavedQueryPyApi($id: ID!) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have schema checker that expects the query to be in the first line

Copy link
Contributor

@dubininsergey dubininsergey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me, works fine locally 👍
Would be great to refactor it, but no-blocker

Comment on lines 241 to 242
print('export execution')
print(client)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we cleanup this prints and leverage logger.* where needed? Eg. logger.debug or logger.info

Comment on lines +711 to +715
res = res[mutation_name]
task_id = res["taskId"]
user: User = self.client.get_user()
tasks: List[Task] = list(
user.created_tasks(where=Entity.Task.uid == task_id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor suggestion: these 3 export_v2 methods could be moved to a separate dedicated service like ExportService. This service could have 3 methods: export_dataset, export_data_row and export_slice. And then these methods could be called in the corresponding classes (Dataset, DataRow and Slice).

This way you would be able to reuse the code parts that are currently duplicated in Dataset.export_v2, DataRow.export_v2 and Slice.export_v2: fetching the task object or building and calling exportDataRowsInCatalog(though it's duplicated only in DataRows and Dataset classes).

Besides reducing code duplication it would increase coherence of the code in ExportService. Also Datasetclass will get to 723 lines long size with this change, putting the most of logic into ExportService would avoid inflating the modules' sizes

Copy link
Contributor Author

@mnoszczak mnoszczak Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my thought as well — will refactor this as part of SDK v4 initiative that aims to use REST endpoints for SDK instead (we'll need backend changes to facilitate this).

@mnoszczak mnoszczak requested a review from dubininsergey March 20, 2023 12:48
@dubininsergey dubininsergey self-requested a review March 20, 2023 13:36
Copy link
Contributor

@tytalus tytalus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mnoszczak mnoszczak merged commit f9e4457 into develop Mar 20, 2023
@mnoszczak mnoszczak deleted the mno/al-5278 branch March 20, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants