-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Update train.py #543
base: main
Are you sure you want to change the base?
Update train.py #543
Conversation
for more information, see https://pre-commit.ci
os.environ.pop("SLURM_NTASKS_PER_NODE", None) | ||
# Set environment variables | ||
os.environ["USE_LIBUV"] = "0" | ||
os.environ["SLURM_NTASKS"] = os.environ.get("SLURM_NTASKS", None) |
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 remove line 16-18
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.
yes if we don’t need to handle SLURM-related environment variables.
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 don't need that, it should work fine in most slurm envs.
Also... I don't see there is a need to make all first-letter of comments uppercase... |
@@ -1,8 +1,6 @@ | |||
import os | |||
|
|||
os.environ["USE_LIBUV"] = "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.
libuv should be in front of everything
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.
Yeah so that environment variable is set before any libraries that might depend on it are imported.
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.
It should be set before pytorch, so at least before lightning / DDP imported
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.
Gotcha!
if there will be anything else let me know i will try!👍 |
this PR is primarily focused on improving code quality and robustness. The improvements include better environment variable handling, enhanced type hinting, more descriptive logging, robust error handling, configuration validation, and improved code readability.
ThankYou,
RAJAT MISHRA.