Skip to content

Conversation

@kannwism
Copy link
Contributor

No description provided.

@kannwism kannwism requested a review from Makuh17 August 22, 2024 12:28
Copy link
Contributor

@Makuh17 Makuh17 left a comment

Choose a reason for hiding this comment

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

I dont really understand a lot of what is going on, but looks fine to me

result = self.get_status()
if result.state == "Completed":
break
wait_time *= 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to exponentially wait more and more? Is that smart? I guess it is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's what it is supposed to do for now. Maybe it should be capped to a maximum of once every 10 minutes or so ^^

:rtype: dict
"""
# return self.openapi_service.order_api.order_get_order_results_get(self.order_id) # throws error
raise NotImplementedError("Currently not supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

lol

Comment on lines +52 to +65
order_model = CreateOrderModel(
orderName=self._name,
workflow=CreateOrderModelWorkflow(self._workflow.to_model()),
userFilters=[],
referee=CreateOrderModelReferee(self._referee.to_model()),
)

result = self._openapi_service.order_api.order_create_post(
create_order_model=order_model
)

self.order_id = result.order_id
self._dataset = RapidataDataset(result.dataset_id, self._openapi_service)
order = RapidataOrder(order_id=self.order_id, dataset=self._dataset, openapi_service=self._openapi_service)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this get much more involved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this got moved from the RapidataOrder class, since this now requires the OrderID to be present

Comment on lines -19 to -29
def k_factor(self, k_factor: int):
self._k_factor = k_factor
return self

def match_size(self, match_size: int):
self._match_size = match_size
return self

def matches_until_completed(self, matches_until_completed: int):
self._matches_until_completed = matches_until_completed
return self
Copy link
Contributor

Choose a reason for hiding this comment

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

So now these are not set by default? Do they default to something on the backend?

self,
client_id: str,
client_secret: str,
endpoint: str = "https://api.rapidata.ai",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be hardcoded?

Copy link
Contributor Author

@kannwism kannwism Aug 22, 2024

Choose a reason for hiding this comment

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

it's a sensible default value I'd say (if it was correct lol)

Comment on lines +11 to +23
class TestClassifyOrder(unittest.TestCase):

def setUp(self):
self.rapi = setup_client()

def test_classify_order(self):
new_classify_order(self.rapi)

def test_free_text_input_order(self):
new_free_text_order(self.rapi)

def test_compare_order(self):
new_compare_order(self.rapi)
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any asserts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but the python client validates the call and the response, so if either the validation or the backend complains, the test will fail.

Base automatically changed from mari/integrate-openapi to main August 22, 2024 14:45
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