Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Fixes train_model worldlogging for multitask with mutators. #4414

Merged
merged 2 commits into from
Mar 17, 2022

Conversation

kauterry
Copy link
Contributor

Patch description

Worldlogging is broken in the train_model script in the multitask setting with mutators bcoz world.getID() omits the mutators and doesn't match the task name.

Testing steps

pytest tests/test_train_model.py

Other information

"""
with testing_utils.tempdir() as tmpdir:
log_report = os.path.join(tmpdir, 'world_logs.jsonl')
multitask = 'integration_tests:mutators=flatten,integration_tests:ReverseTeacher:mutator=reverse'
Copy link
Contributor

@jxmsML jxmsML Mar 15, 2022

Choose a reason for hiding this comment

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

I wonder if we can test this with integration_tests:mutators=flatten,integration_tests:mutator=reverse to highlight that you wanted to test if world logs works for exact same teacher except for mutators

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just trying to convey that the two tasks can be anything, it's the number of tasks that matter. So it works for any two tasks.

Copy link
Contributor

@jxmsML jxmsML left a comment

Choose a reason for hiding this comment

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

LGTM! suggestion on the test

Copy link
Contributor

@jxmsML jxmsML left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the new test case

@kauterry kauterry merged commit d6773a0 into main Mar 17, 2022
@kauterry kauterry deleted the krs_train_model_bug branch March 17, 2022 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants