Disable flaky twitter bots dataset loading test. #3439
Conversation
| # DISABLED: Flaky for tests, probably due to the dataset size. | ||
| # # Test loading dataset without 'split' and 'Unnamed: 0' columns in config. | ||
| # twitter_bots_config = ludwig.datasets._get_dataset_config("twitter_bots") | ||
| # assert isinstance(twitter_bots_config, DatasetConfig) | ||
|
|
||
| # twitter_bots_dataset = ludwig.datasets.get_dataset("twitter_bots", cache_dir=tmpdir) | ||
| # assert isinstance(twitter_bots_dataset, DatasetLoader) | ||
| # df = twitter_bots_dataset.load() | ||
| # assert df is not None | ||
| # assert len(df.columns) == 22 # Expected number of columns in Twitter bots dataset including split column. |
There was a problem hiding this comment.
Should we skip the test? Or is this fine?
There was a problem hiding this comment.
There's an upper half of the test that tests the basic mechanics of loading a dataset that hasn't been flake, so still want to keep the test overall.
The twitter bots block additionally checks that loading works when there's a pre-set split column, which seems incremental. My feeling is that it's not worth keeping it enabled since we already have the first test.
There was a problem hiding this comment.
Do you happen to remember what the purpose/intention of this test was? I remember we added this twitter bots component in the last year but I can't remember the reason for it. Just want to make sure it's ok to disable!
Going to temporarily approve for now
There was a problem hiding this comment.
Looks like this test was added in https://github.com/ludwig-ai/ludwig/pull/3285/files by @connor-mccorm to verify loading when there's a pre-set split column. Removing this test seems low risk to me, but it's certainly checking behavior that isn't covered by the first test.
Since the test hasn't failed for a few CI runs now, my guess is that the original test failed because of a momentary Kaggle outage, similar to how our HF tests used to fail whenever there was an HF outage.
I'd be more inclined to merge this PR if we've seen the test fail multiple times, but it's just been once, this PR could be an overreaction.
|
Seems like this test has failed again in https://github.com/ludwig-ai/ludwig/actions/runs/5836017733/job/15828730546. Re-opening. |
No description provided.