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

Put extra arg test back #1641

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

Put extra arg test back #1641

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e68903e
update
XiaohanZhangCMU Nov 5, 2024
27add58
update
XiaohanZhangCMU Nov 5, 2024
4922cd1
update
XiaohanZhangCMU Nov 6, 2024
79d2f72
Merge branch 'main' into xiaohan/enable_extra_arg_test
dakinggg Nov 6, 2024
ffc9390
Merge branch 'main' into xiaohan/enable_extra_arg_test
XiaohanZhangCMU Nov 6, 2024
fda9c12
Merge branch 'main' into xiaohan/enable_extra_arg_test
milocress Nov 13, 2024
f7942f4
Merge branch 'main' into xiaohan/enable_extra_arg_test
milocress Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions tests/data/test_dataloader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1517,8 +1517,9 @@ def test_ft_dataloader_with_extra_keys():
).dataloader

# TODO: Change this back to xfail after figuring out why it caused CI to hang
@pytest.mark.skip
@pytest.mark.xfail
def test_text_dataloader_with_extra_keys():
from streaming.base.constant import BARRIER_FILELOCK, CACHE_FILELOCK
max_seq_len = 1024
cfg = {
'dataset': {
Expand Down Expand Up @@ -1548,16 +1549,18 @@ def test_text_dataloader_with_extra_keys():
)

device_batch_size = 2
def custom_stat_mock(path: Any):
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is some really intense patching, are you sure this test can't be made simpler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, should be, but I was not able to come up with one. Any suggestions? I basically need
os.path.exists(filelocks) to behave normally, and other os.stat to return a mock stat when testing with private-streaming.

if BARRIER_FILELOCK in path or CACHE_FILELOCK in path:
return original_os_stat(path)
return MagicMock(st_size=1024, st_mode=33188) # Mock regular file attributes

mock_stat = MagicMock()
mock_stat.st_size = 1024 # Mock st_size with a desired value
mock_stat.st_mode = 33188 # Regular file mode for Unix-based systems
original_os_stat = os.stat

#with patch('streaming.base.stream.get_shards', return_value=None):
with patch('os.makedirs'), \
patch('builtins.open', new_callable=mock_open, read_data='{"version": 2, "shards": []}'), \
patch('json.load') as mock_json_load, \
patch('os.stat', return_value=mock_stat), \
patch('os.stat', side_effect=custom_stat_mock), \
patch('torch.distributed.is_available', return_value=True), \
patch('torch.distributed.is_initialized', return_value=True), \
patch('torch.distributed.broadcast_object_list'), \
Expand Down
Loading