fixing version issue #90
Conversation
| import tomli | ||
|
|
||
|
|
||
| def get_version_from_pyproject(): | ||
| """Get the code version from pyproject.toml file.""" | ||
| with open("pyproject.toml", "rb") as f: | ||
| relative_path = Path(__file__).parents[4] / "pyproject.toml" |
There was a problem hiding this comment.
Isn't the version in the pyproject installed into the package itself? This seems like not the right way to get the version. Chat gpt says you can do this:
import importlib.metadata
def get_package_version(package_name: str) -> str:
return importlib.metadata.version(package_name)
Example usage
package_name = "bionemo.scdl"
version = get_package_version(package_name)
print(f"The installed version of {package_name} is {version}")
There was a problem hiding this comment.
Yeah that seems like a legit suggestion from chatgpt. Prob the ideal way to do this. If it doesn't work for sub packages then document that in a comment and go ahead with the your current approach. Thanks again! https://docs.python.org/3/library/importlib.metadata.html
There was a problem hiding this comment.
+1 on this suggestion. What do we need a version tag for? It would be ideal to move away from hard-coded versions anywhere in the package, and use setuptools-scm to pull the version from github tags.
There was a problem hiding this comment.
@pstjohn that's cool. Let's plan that out later though for the auto-versioning. For now I think the focus of this PR should just be on using the correct mechanism (looking in package metadata) for the version. Your solution should also put the version in the metadata so I think if she goes with the importlib.metadata route it will work for getting version info regardless of how it's populated.
There was a problem hiding this comment.
So for this use case, getting the version is for logging purposes and stored into the metadata of the processed dataset as far as I can tell. This tells us what version of the tool was used to make the dataset. Sorry not sure if that's what you were asking or not @pstjohn.
There was a problem hiding this comment.
pushed the fix.
|
/build-ci |
polinabinder1
requested review from
pstjohn
and removed request for
skothenhill-nv
| @@ -13,11 +13,10 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| import tomli | |||
| import importlib.metadata | |||
|
|
|||
|
|
|||
| def get_version_from_pyproject(): | |||
There was a problem hiding this comment.
Since you have to update this anyways, we could move this to a general place, like bionemo-core/src/bionemo/core/utils/version_utils.py and make the package_name a required arg, like def get_version_from_package(package_name:str) -> str and just have it be the one liner return importlib.metadata.version(package_name) that you have now. Then you could delete this file, and change the place it's used to import/call that thing. This a smaller suggestion for sure, I'm happy with it either way.
There was a problem hiding this comment.
it's a one-liner anyways. Why not just call importlib.metadata.version(package_name) anywhere you need the version, and delete this file?
|
i'd rather you just |
|
/build-ci |
|
/build-ci |
No description provided.