-
Notifications
You must be signed in to change notification settings - Fork 135
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
Readme, pyproject metadata and makefile fixes in noop and filter. #240
Conversation
…amples, add metadata to pyproject.toml for the libraries. Signed-off-by: David Wood <[email protected]>
run-local-sample: .transforms.run-local-sample | ||
|
||
run-s3-sample: .transforms.run-s3-sample | ||
run-s3-sample: .transforms.run-s3-ray-sample | ||
|
||
minio-start: .minio-start | ||
|
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.
Can we fix minio-start as well. Currently it does not copy additional files beyond input
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.
can we make this a separate PR. its pretty orthogonal to these changes
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.
We can, but, we talked about it several times and its still not there
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.
Submit an issue and don't poke arbitrary fingers at random PRs over this. Make an issue and fix it in a separate PR. This PR is about something else! We can't start holding up PRs over random unrelated (and unreported) issues.
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.
Well, minio-start
is currently broken. In addition to what I noted here before its also always getting data from ray directory. So I am not convinced we should merge a PR relying on the broken things
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.
The issue #65 is covering this problem, but it has been marked as fixed. Do we need to re-open that issue again?
@@ -2,6 +2,7 @@ | |||
name = "data_prep_toolkit" | |||
version = "0.2.0.dev6" | |||
requires-python = ">=3.10" | |||
keywords = ["data", "data preprocessing", "data preparation", "llm", "generative", "ai", "fine-tuning", "llmapps" ] | |||
description = "Data Preparation Toolkit Library" | |||
license = {text = "Apache-2.0"} |
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.
Should it specify Python in the description and maybe name??
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.
In the future, please make a suggestion to your liking. How about
Data Preparation Toolkit for Python
However, this seems a bit redundant since pypi sort of implies python.
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.
We have 3 different libraries - one for Ray, one for Spark and one for ? nothing?
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 think the name of the library, Data Preparation Toolkit Library
is clear enough.
run-local-sample: .transforms.run-local-sample | ||
|
||
run-s3-sample: .transforms.run-s3-sample | ||
run-s3-sample: .transforms.run-s3-ray-sample | ||
|
||
minio-start: .minio-start | ||
|
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.
We can, but, we talked about it several times and its still not there
Why are these changes needed?
Update filter and noop readmes, Fix filter Makefiles regard running s…amples, add metadata to pyproject.toml for the libraries.
Related issue number (if any).