-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pbinder/move scdl #76
Conversation
/build-ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a couple comments around packaging, these don't have to block the PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but any reason you need to have this file and the version specified in the pyproject.toml?
Could you instead see if
from importlib.metadata import version
version('bionemo-scdl')
would let you have the version tag in only one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, they should be in the same one. Good point.
sub-packages/bionemo-scdl/setup.py
Outdated
setup( | ||
name="scdl", | ||
version="0.0.1", | ||
packages=find_packages(), | ||
author="NVIDIA Corporation", | ||
author_email="[email protected]", | ||
description="A library for fast dataloading of single cell data.", | ||
long_description="scdl is a library for reading and writing performant datasets from single cell foundation models.", | ||
url="", | ||
classifiers=[ | ||
"Development Status :: 3 - Alpha", | ||
"Intended Audience :: Developers", | ||
"Programming Language :: Python :: 3.10", | ||
"Programming Language :: Python :: 3.11", | ||
], | ||
python_requires=">=3.10", | ||
install_requires=[], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malcolmgreaves -- do we still need a setup.py script? why isn't the pyproject.toml
sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it
/build-ci |
|
||
Alternatively, it can be installed with | ||
```bash | ||
pip install bionemo[scdl] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work? :O
scripts/convert_h5ad_to_scdl.py
Outdated
# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
# SPDX-License-Identifier: LicenseRef-Apache2 | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a duplicate of the module level sub-package/bionemo-scdl/scripts/convert_h5ad_to_scdl.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a brief pass- only thing that stands out is convert_h5ad_to_scdl.py
is defined in two places, other than that LGTM.
/build-ci |
2 similar comments
/build-ci |
/build-ci |
b67ae19
to
6c3747c
Compare
/build-ci |
/build-ci |
/build-ci |
1 similar comment
/build-ci |
Moving the scdl changes over.