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

adding module lookup for building trackers #803

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

clumsy
Copy link
Contributor

@clumsy clumsy commented Dec 19, 2023

Discussed this in #802

TL;DR this adds the ability to reference a module path instead of entry_point for tracker backend as a fallback when entry_point is not found, e.g.:

[torchx.tracker]
my_tracker = torchx.tracker.mlflow.create_tracker

[tracker:my_tracker]
...

Test plan:

  • Added a unit test, the original functionality is there but as a fallback we also check if we can find tracker backend using module lookup.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 19, 2023
@clumsy
Copy link
Contributor Author

clumsy commented Dec 19, 2023

For your consideration, @kurman and @kiukchung.

torchx/tracker/api.py Outdated Show resolved Hide resolved
@clumsy clumsy force-pushed the feature/tracker_module_lookup branch from 9c90c3e to f51a32b Compare January 3, 2024 15:42
@clumsy
Copy link
Contributor Author

clumsy commented Jan 3, 2024

@kiukchung removed hydra usage, following entrypoint syntax. Please let me know if you have further comments.

@clumsy clumsy force-pushed the feature/tracker_module_lookup branch from f51a32b to bc135cf Compare January 3, 2024 19:00
Copy link
Collaborator

@kiukchung kiukchung left a comment

Choose a reason for hiding this comment

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

Thanks for the PR (and removing the hydra dep). Could you update the docs as well?
Its all done via docstrings in torchx/tracker/__init__.py

You'll want to update the Tracker Setup and split out the rest of the docs into entry_point and module registrations.

You can preview the docs locally by:

$ cd docs
$ pip install -r requirements.txt
$ make clean html
$ python -m http.server -d build/0.7.0.dev0/html

Then opening up the docs page on https://localhost:8080

see docs-build CI action for details.

torchx/tracker/api.py Outdated Show resolved Hide resolved
torchx/tracker/api.py Show resolved Hide resolved
@clumsy clumsy force-pushed the feature/tracker_module_lookup branch from bc135cf to bd97b1f Compare January 11, 2024 00:05
@clumsy
Copy link
Contributor Author

clumsy commented Jan 11, 2024

Ok @kiukchung I addressed the comments, updated the documentation. Additionally I extracted the load_module functionality and tests to be separate from entrypoints. I didn't test the documentation rendering yet.

@clumsy clumsy force-pushed the feature/tracker_module_lookup branch from bd97b1f to 063626f Compare January 12, 2024 13:53
@clumsy
Copy link
Contributor Author

clumsy commented Jan 12, 2024

I addressed an unrelated check failure due a broken link in Kubernetes docs. KPF seem to fail for an unrelated reason, @kiukchung.

@clumsy
Copy link
Contributor Author

clumsy commented Jan 24, 2024

@kiukchung do I need to make furhter changes to this PR? I don't have conflicts when fetch/rebasing on top of origin/main locally.

@kiukchung kiukchung merged commit b54df67 into pytorch:main Jan 24, 2024
15 of 22 checks passed
@clumsy clumsy deleted the feature/tracker_module_lookup branch January 24, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants