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

Simplify run training #101

Closed
wants to merge 40 commits into from
Closed

Simplify run training #101

wants to merge 40 commits into from

Conversation

jaidhyani
Copy link
Collaborator

@jaidhyani jaidhyani commented Apr 1, 2024 •
edited
Loading

This sits on top of the train_rework PRs; only the commits from c6fe08c ("Remove unused imports in run_training.py") are really part of this PR. Everything important (before code review) should be here: 53ec272

jaidhyani added 30 commits March 25, 2024 21:46
@jaidhyani
rename "train_step" to "iteration_step"
d596ddc
@jaidhyani
Merge remote-tracking branch 'origin/main' into train_rework
17a4081
@jaidhyani
isolate train_step
ef950aa
@jaidhyani
vscode debugging configs
084d452
@jaidhyani
replace generator with direct indexing for data
b18899a
@jaidhyani
unused import
ef3fb7d
@jaidhyani
get rid of eval_callbacks
62b7045
@jaidhyani
remove unused return value
4fbf375
@jaidhyani
only pass device to train_step
ae6ccc0
@jaidhyani
remember to init wandb when using it!
00cd95c
@jaidhyani
factor accumulate_gradients out + testing for train_step
e484532
@jaidhyani
fixed incorrect num_batches in test that could give cause negative
ca70939
@jaidhyani
conditional wandb silencing in init_wandb
1b220f8
@jaidhyani
configurable train/validation datasets
ccce615
@jaidhyani
fix docstring on accumulate_gradients
33d8a20
@jaidhyani
factor out some setup boilerplate from run_training
43df804
@jaidhyani
rm new_best_val_loss, simplify save_checkpoint logic slightly
50f2ae9
@jaidhyani
rm CheckpointData, minor wandb log tweaks
4df129d
@jaidhyani
rm IterationParams
eba9d37
@jaidhyani
unused import
f79b561
@jaidhyani
resume_from_path
b6f83fd
@jaidhyani
huggingface - save to iter_N revision on repo
c6450d1
@jaidhyani
rm model_config
8b7aef8
@jaidhyani
manual checkpoint iters
cfd1490
@jaidhyani
512
b4e86ee
@jaidhyani
config cleanup
0814b1c
@jaidhyani
unused import
628e35e
@jaidhyani
fix end2end demo
1e1bd6a
@jaidhyani
wip
7e72b23
@jaidhyani
CR: test deterministic train step
4b37f5f
@jaidhyani jaidhyani marked this pull request as ready for review April 1, 2024 08:26
@jaidhyani jaidhyani requested a review from jettjaniak April 1, 2024 08:27
This was linked to issues Apr 1, 2024
Assorted deletions/simplicatications #82
Closed
flatten + simplify training script arguments #90
Closed
@jaidhyani jaidhyani force-pushed the simplify_run_training branch from 8ec1043 to 81a353d Compare April 1, 2024 08:32
@jettjaniak
Copy link
Contributor

I think we should always rebase instead of merge and point PRs to the actual target branch. This one needs to be rebased now on top of main, but there are some conflicts I don't know what to do about

jettjaniak
jettjaniak requested changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@jettjaniak jettjaniak left a comment

Choose a reason for hiding this comment

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

rebase

@jaidhyani
Copy link
Collaborator Author

I think we should always rebase instead of merge and point PRs to the actual target branch. This one needs to be rebased now on top of main, but there are some conflicts I don't know what to do about

The tension here is with keeping PRs from getting huge while responding to code review. So you run into issues when you:

  1. Make a bunch of diffs
  2. Make PR-1 from those diffs
  3. Continue working on more diffs on top of PR-1 (but not part of PR-1 because that would lead to the PR getting way too big and unwieldy and unfocused and hard to review)
  4. Make a new PR with the diffs that have accumulated on top of PR-1
  5. Get feedback on PR-1 and make updates to PR-1

At this point, rebasing PR-2 will require resolving conflicts for every diff that's been potentially touched by the code review diffs on PR-1, and this typically ends up being a huge and pointless waste of time, essentially resolving the same conflicts over and over again. Rebasing and fast-forwarding is great when you can do it, but this kind of situation is the exact reason why it's sometimes preferable to do a single merge and deal with the conflicts in one go.

@jettjaniak jettjaniak mentioned this pull request Apr 2, 2024
Merged
@jettjaniak
Copy link
Contributor

I agree with some things you're saying, I just think this is all solvable. Would love to discuss at our next meeting. In the meantime:

I did git rebase -i --onto main COMMIT where COMMIT is one commit before

c6fe08c ("Remove unused imports in run_training.py")

I used the -i option just to drop the last commit that is a merge of main

The resulting branch is here #104, LMK if this looks good

@jaidhyani
Copy link
Collaborator Author

Replaced by #104

@jaidhyani jaidhyani closed this Apr 2, 2024
@jettjaniak jettjaniak deleted the simplify_run_training branch May 22, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jettjaniak jettjaniak jettjaniak requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flatten + simplify training script arguments Assorted deletions/simplicatications
2 participants
@jaidhyani @jettjaniak