allegroai / clearml

ClearML - Auto-Magical CI/CD to streamline your AI workload. Experiment Management, Data Management, Pipeline, Orchestration, Scheduling & Serving in one MLOps/LLMOps solution
https://clear.ml/docs
Apache License 2.0
5.61k stars 651 forks source link

Fix dataset.list dataset() partial match fix for special chars #1077

Closed natephysics closed 1 year ago

natephysics commented 1 year ago

Related Issue \ discussion

https://github.com/allegroai/clearml/issues/1076

Patch Description

This patch fixes Task._query_task() by escaping the task_name attribute passed to the function. Without this, task_names passed with special characters would break the search.

Testing Instructions

See: https://github.com/allegroai/clearml/issues/1076

jkhenning commented 1 year ago

Hi @natephysics, methods using Task._query_tasks() such as Task.get_tasks() and Task.query_tasks() actually depend on the string not being escaped since they explicitly state that the task_name can be a regex... Perhaps the right way to address it is to add a note to their docstring, reminding users that in case they do not with to use a regex, they should escape the string?

natephysics commented 1 year ago

That makes sense. I actually asked in the discussion if it should be addressed at that level. This was mainly to address the issue of it not being clear that Dataset.list_dataset() partial name matching required escaping as well. Is this also one you want to preserve regex formatting for? If so it would be good to document this as it was a point of confusion.

jkhenning commented 1 year ago

Yes, this is required for internal flexibility as well. I suggest we'll improve the documentation in the docstrings of affected methods - would you like to cancel this PR or change it?

natephysics commented 1 year ago

I'll close it.