Skip to content
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

Misc changes #230

Merged
merged 12 commits into from
Dec 19, 2024
Merged

Misc changes #230

merged 12 commits into from
Dec 19, 2024

Conversation

mercuryseries
Copy link
Contributor

@mercuryseries mercuryseries commented Dec 9, 2024

Changelogs


Checklist:

  • Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
  • Add tests to cover the fixed bug(s) or the newly introduced feature(s) (if appropriate).
  • Update the API documentation if a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs above.
  • If possible, assign one of the following labels to the PR: feature, fix, chore, documentation or test (or ask a maintainer to do it for you).

discussion related to that PR

@mercuryseries mercuryseries self-assigned this Dec 9, 2024
@mercuryseries mercuryseries added chore fix Annotates any PR that fixes bugs and removed chore labels Dec 9, 2024
@mercuryseries mercuryseries marked this pull request as ready for review December 9, 2024 14:42
…tifacts.

Co-authored-by: Julien St-Laurent <jstlaurent@users.noreply.github.com>
Copy link
Contributor

@jstlaurent jstlaurent left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@cwognum cwognum left a comment

Choose a reason for hiding this comment

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

Thanks, @mercuryseries !

polaris/hub/client.py Outdated Show resolved Hide resolved
polaris/hub/client.py Outdated Show resolved Hide resolved
polaris/hub/client.py Outdated Show resolved Hide resolved
Co-authored-by: Cas Wognum <cwognum@users.noreply.github.com>
Copy link
Collaborator

@cwognum cwognum left a comment

Choose a reason for hiding this comment

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

Thanks, @mercuryseries !

Why the many string conversions? The test cases weren't failing, so I assume it's to be consistent with typing? If so, what do you think about instead extending the type hints to also accept a Path ? That seems to actually be the more appropriate type here anyways.

polaris/hub/client.py Outdated Show resolved Hide resolved
polaris/hub/client.py Outdated Show resolved Hide resolved
polaris/hub/client.py Outdated Show resolved Hide resolved
polaris/hub/client.py Outdated Show resolved Hide resolved
polaris/hub/client.py Outdated Show resolved Hide resolved
Co-authored-by: Cas Wognum <cwognum@users.noreply.github.com>
@mercuryseries
Copy link
Contributor Author

Why the many string conversions? The test cases weren't failing, so I assume it's to be consistent with typing? If so, what do you think about instead extending the type hints to also accept a Path ? That seems to actually be the more appropriate type here anyways.

Yes, I agree that extending the type hints to accept Path would be more appropriate. I considered doing that during the refactoring, but decided to hold off because the datamol.utils.fs.join function, which is used in some places, currently accepts a variable number of str: https://docs.datamol.io/0.7.15/api/datamol.utils.html#datamol.utils.fs.join. While it does internally convert input paths to str if you pass them in, the type hints don't reflect that. So, to properly support Path inputs, we'd need to update the type annotations of join and possibly other related functions.

@cwognum
Copy link
Collaborator

cwognum commented Dec 19, 2024

@jstlaurent has repeatedly told me to not (ab)use Datamol for its filesystem... Old habits die hard... The use of datamol.utils.fs.join should be considered legacy code. Ideally, we should replace this with methods such as os.path.join (for local paths) or urllib.parse.urljoin (for URLs). I'm okay with leaving that outside the scope of this PR though.

@mercuryseries mercuryseries merged commit f72ff53 into main Dec 19, 2024
10 checks passed
@jstlaurent jstlaurent deleted the misc-changes branch January 13, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Annotates any PR that fixes bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize artifact_type typing in _get_v1_dataset, _upload_v1_dataset and _upload_benchmark
3 participants