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.

Sign up for GitHub

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

Jump to bottom

add cli access #25

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add cli access #25

wants to merge 6 commits into from

Conversation

observingClouds
Copy link
Contributor

@observingClouds observingClouds commented Oct 3, 2024 •
edited
Loading

This adds CLI access such that one can call mllam-data-prep directly from the command line, like mllam_data_prep my_awesome_dataset.yaml

Note: Tests currently fail as this depends on #27 to be merged. It also depends on #26.

@observingClouds observingClouds marked this pull request as draft October 3, 2024 10:49
@observingClouds observingClouds marked this pull request as ready for review October 3, 2024 12:19
leifdenby
leifdenby reviewed Oct 30, 2024
View reviewed changes
tests/test_distributed.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

this test is pure elegance, nice!

@leifdenby
Copy link
Member

#27 has been merged at #26 is ready for you to merge :) once they have gone in I can have a go at reviewing this properly

@observingClouds observingClouds marked this pull request as draft October 30, 2024 21:36
@observingClouds observingClouds marked this pull request as ready for review October 31, 2024 11:43
leifdenby
leifdenby requested changes Nov 7, 2024
View reviewed changes
Copy link
Member

@leifdenby leifdenby left a comment

Choose a reason for hiding this comment

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

Only minor suggestion is to make the changelog entry a bit more descriptive.

Should we try and get this into the next release? If so we should add this to the community roadmap for the next release

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- add support for CLI calls. ![\25](https://github.com/mllam/mllam-data-prep/pull/25).
Copy link
Member

Choose a reason for hiding this comment

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

maybe it would be clearer to say "add support for calling cli from python function and add tests for cli with/without dask.distributed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I updated it. Please check.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we should just write what the CLI call is now? Is it still python -m ...?

Maybe the changelog should then say

add new CLI call option ... (so that python -m mllam_data_prep ... isn't needed)

or something like that?

leifdenby
leifdenby approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@leifdenby leifdenby left a comment

Choose a reason for hiding this comment

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

Perfecto!

Ready to merge then. But we should probably wait and put this on the roadmap before we merge

@leifdenby
Copy link
Member

I forgot to propose this so that it got on the roadmap, sorry @observingClouds! I was going to try and sneak it into v0.5.0 that I am about to release, but I can't get the cli command to work:
Screenshot 2024-11-20 at 19 20 34

any idea what I could be doing wrong?

@leifdenby
Copy link
Member

For now I suggest we propose this for v0.6.0 so that I can go ahead with merging v0.5.0

@leifdenby leifdenby added this to the v0.6.0 (proposed) milestone Nov 20, 2024
@observingClouds
Copy link
Contributor Author

Could it be that you need pdm run mllam_data_prep?

@leifdenby
Copy link
Member

Could it be that you need pdm run mllam_data_prep?

Yes of course! DUH! Sorry and thank you. Ok, this works for me then :)

@leifdenby leifdenby self-requested a review November 21, 2024 07:41
@leifdenby leifdenby modified the milestones: v0.6.0 (proposed), v0.6.0 Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leifdenby leifdenby leifdenby approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@observingClouds @leifdenby