Refactoring Catalog.get_dataset()
#1249
Conversation
Reviewer's GuideThis PR refactors the dataset lookup API by changing Sequence diagram for dataset lookup with new get_dataset signaturesequenceDiagram
participant Caller
participant Catalog
participant Metastore
Caller->>Catalog: get_dataset(name, namespace_name, project_name)
Catalog->>Metastore: get_dataset(name, namespace_name, project_name)
Metastore-->>Catalog: DatasetRecord
Catalog-->>Caller: DatasetRecord
Class diagram for refactored Catalog.get_dataset and Metastore.get_datasetclassDiagram
class Catalog {
+get_dataset(name: str, namespace_name: Optional[str] = None, project_name: Optional[str] = None) DatasetRecord
}
class Metastore {
+get_dataset(name: str, namespace_name: Optional[str] = None, project_name: Optional[str] = None, conn=None) DatasetRecord
}
Catalog --> Metastore : uses
class Project {
+name: str
+namespace: Namespace
}
class Namespace {
+name: str
}
class DatasetRecord {
+name: str
+project: Project
+latest_version: str
+has_version(version: str): bool
}
Metastore --> Project
Project --> Namespace
Catalog --> DatasetRecord
Metastore --> DatasetRecord
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ilongin - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/datachain/data_storage/metastore.py:921` </location>
<code_context>
) -> DatasetRecord:
"""Creates new dataset."""
- project_id = project_id or self.default_project.id
+ if not project_id:
+ project = self.default_project
+ else:
+ project = self.get_project_by_id(project_id)
query = self._datasets_insert().values(
</code_context>
<issue_to_address>
Logic for determining project_id in create_dataset may be redundant.
Since project.id is always used in the insert, fetching the full project object when project_id is already provided may be unnecessary. Consider simplifying to avoid the extra lookup.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if not project_id: | ||
| project = self.default_project | ||
| else: | ||
| project = self.get_project_by_id(project_id) |
There was a problem hiding this comment.
nitpick: Logic for determining project_id in create_dataset may be redundant.
Since project.id is always used in the insert, fetching the full project object when project_id is already provided may be unnecessary. Consider simplifying to avoid the extra lookup.
| for r in catalog.get_dataset("dogs_custom_columns").get_version("1.0.0").preview: | ||
| assert isinstance(r.get("file__last_modified"), str) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| ds = self._parse_dataset(self.db.execute(query, conn=conn)) | ||
| if not ds: | ||
| raise DatasetNotFoundError( | ||
| f"Dataset {name} not found in project with id {project_id}" | ||
| f"Dataset {name} not found in namespace {namespace_name}" | ||
| f" and project {project_name}" | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Lift code into else after jump in control flow (
reintroduce-else) - Swap if/else branches (
swap-if-else-branches)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1249 +/- ##
==========================================
+ Coverage 88.71% 88.73% +0.01%
==========================================
Files 155 155
Lines 14130 14123 -7
Branches 1993 1994 +1
==========================================
- Hits 12536 12532 -4
+ Misses 1127 1124 -3
Partials 467 467
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Deploying datachain-documentation with
|
| Latest commit: |
7d33924
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ecc011cf.datachain-documentation.pages.dev |
| Branch Preview URL: | https://ilongin-1237-refactore-get-d.datachain-documentation.pages.dev |
dmpetrov
left a comment
dmpetrov
left a comment
There was a problem hiding this comment.
Let's use fully qualified dataset names instead of 3 params: name, namespace, project.
| self, | ||
| name: str, | ||
| namespace_name: Optional[str] = None, | ||
| project_name: Optional[str] = None, |
There was a problem hiding this comment.
Let's use full project name instead of additional params - ns1.ns2.ds3
There was a problem hiding this comment.
This is internal API where we already have it split this into 3 parts. In public API we use fully qualified name. Honestly I wouldn't change this atm as it would again require me some refactoring which would take time and we already have this kind of signature in other internal methods so not much would be changed. WDYT?
There was a problem hiding this comment.
Let's approve to move fast. But we are accumulating tech depth using different notations in internal and external APIs. Please create a followup issue.
There was a problem hiding this comment.
Just an idea: we can store full name in dataset table as dataset name, this will simplify everything and we don't need anymore to pass namespace and project name everywhere.
There was a problem hiding this comment.
We don't need to do that as dataset table already have connection to project table which has connection to namespaces table, so it's normalized. Also, when fetching dataset from table object DatasetRecord already has Project object in it and property method full_name which returns namespace, project and dataset name so it's all ok.
We need explicit namespace / project when we are fetching dataset (as here) or saving the new one in DB. The only question is should we everywhere have it split into 3 arguments, as now, or should we have just dataset name which must, by convention, have namespace and project embedded in it, e.g namespace.project.dataset. This is what we already have in public API but not in internal ones.
dreadatour
left a comment
dreadatour
left a comment
There was a problem hiding this comment.
Looks good to me overall 👍
| namespace_name = namespace_name or self.metastore.default_namespace_name | ||
| project_name = project_name or self.metastore.default_project_name |
There was a problem hiding this comment.
Just wonder, if it make sense if project_name is set with namespace_name is empty (default one) and vise versa (namespace_name is set, but project_name is default one).
I assume yes, it make sense in some cases, but it also may leads to some weird behavior. Taking the full name (ns1.ns2.ds3 as @dmpetrov suggests below) may solve this issue (it will not be possible to set namespace name only, without setting project name). Or may be I am wrong and this is a nice feature to have. Just something to keep in mind and think about, may be later.
Note: to be honest I am still confused with namespace and project — which one goes first 😥
There was a problem hiding this comment.
It def makes sense to have just project name set, as then default namespace will be used, but you are right about vice versa .. if someone just sets namespace name then default project will be used which can be little bit weird. I'm not 100% sure if that's ok or not TBH but for now we allow all combinations.
It's namespace.project.dataset :D That's hierarchy in DB as well.
4 participants
Refactoring
Catalog.get_dataset()to acceptproject_nameandnamespace_nameinstead of already instantiatedProject. This is because in more than one place we have project and namespace name available, but don't have wholeProjectinstance and we needed to fetch it from DB because of that which was basically +1 SQL command that could've been avoided.Summary by Sourcery
Refactor dataset lookup to use namespace and project names instead of passing Project objects throughout the codebase
Enhancements: