Review test readme and define best practices

[miguelgfierro]

Description

• Added some comments on the test README
• Defined some best practices when assert is True

The reasoning is to make the tests more explicit. For example, if a function returns True, we want to make sure the assert checks for True and not for a truthy value.

Example:

result = True
assert result is True  # Passes because result is exactly True

result = 1
assert result  # Passes because 1 is a truthy value
assert result is True  # Fails because 1 is not the exact True object

Related Issues

References

Checklist:

• I have followed the contribution guidelines and code style for this project.
• I have added tests covering my contributions.
• I have updated the documentation accordingly.
• I have signed the commits, e.g. git commit -s -m "your commit message".
• This PR is being made to staging branch AND NOT TO main branch.

[miguelgfierro]

It seems I broke something when I renamed one of the folders

            "code": "ValidationError",
            "details": [
                ***
                    "code": "Invalid",
                    "message": "Failed to create snapshot. File node tests/smoke/recommenders/models/__init__.py was specified but does not appear in the list of uploaded files. Please delete the snapshot cache and resubmit. Snapshot cache is located in .projectcontent folder, typically under .azureml folder in your home folder",
                    "target": "nodes"
                ***
            ],
            "message": "Failed to create snapshot. File node tests/smoke/recommenders/models/__init__.py was specified but does not appear in the list of uploaded files. Please delete the snapshot cache and resubmit. Snapshot cache is located in .projectcontent folder, typically under .azureml folder in your home folder",
            "target": "nodes"
        ***,

"error": ***
        "message": "***\n    \"error_details\": ***\n        \"componentName\": \"project\",\n        \"correlation\": ***\n            \"operation\": \"d7b079d7bbac3dd9c21f12b04[151](https://github.com/recommenders-team/recommenders/actions/runs/9918504884/job/27403242773?pr=2130#step:3:158)b40d\",\n            \"request\": \"cd3d47291294bee4\"\n        ***,\n        \"environment\": \"eastus\",\n        \"error\": ***\n            \"code\": \"ValidationError\",\n            \"details\": [\n                ***\n                    \"code\": \"Invalid\",\n                    \"message\": \"Failed to create snapshot. File node tests/smoke/recommenders/models/__init__.py was specified but does not appear in the list of uploaded files. Please delete the snapshot cache and resubmit. Snapshot cache is located in .projectcontent folder, typically under .azureml folder in your home folder\",\n                    \"target\": \"nodes\"\n                ***\n            ],\n            \"message\": \"Failed to create snapshot. File node tests/smoke/recommenders/models/__init__.py was specified but does not appear in the list of uploaded files. Please delete the snapshot cache and resubmit. Snapshot cache is located in .projectcontent folder, typically under .azureml folder in your home folder\",\n            \"target\": \"nodes\"\n        ***,\n        \"location\": \"eastus\",\n        \"statusCode\": 400,\n        \"time\": \"2024-07-13T08:14:28.3166442+00:00\"\n    ***,\n    \"status_code\": 400,\n    \"url\":

[miguelgfierro]

@SimonYansenZhao do you know what is this snapshot cache?

[SimonYansenZhao]

@miguelgfierro After research, I think a snapshot is just a copy of recommenders. In the testing workflow, the repo is copied onto the GitHub CICD VM by the action checkout:

recommenders/.github/workflows/azureml-unit-tests.yml

Lines 38 to 39 in d333a0d

	- name: Check out repository code
	uses: actions/checkout@v4

then the copy is uploaded onto the AzureML compute by the Azure ML SDK as a snapshot. I am not sure why the __init__.py file wasn't uploaded. But it is now uploaded into the snapshot cache after I disable snapshot and re-enable. In other words, as the error suggested, we need to delete the snapshot cache directory. Disabling snapshot seemed to have the effect of removing the cache directory, and then re-enabling snapshot makes sure recommenders repo is uploaded onto the AzureML compute. It looks like a bug of AzureML.

Two solutions that may permanently resolve this problem are:

1. Add code to delete the snapshot cache directory every time when uploading the copy of recommenders.
2. Clone recommenders directly in the docker image used for AzureML compute.

[miguelgfierro]

Possible solutions:

• https://stackoverflow.com/questions/66045485/azure-ml-how-do-i-fix-this-snapshot-exception
• https://azure.github.io/azureml-cheatsheets/docs/cheatsheets/python/v1/run-history/#snapshot

[SimonYansenZhao]

@miguelgfierro We got new errors: https://github.com/recommenders-team/recommenders/actions/runs/9954661853/job/27500793010

______________________________ test_download_path ______________________________

    def test_download_path():
        # Check that the temporal path is created and deleted
        with download_path() as path:
            assert os.path.isdir(path) is True
        assert os.path.isdir(path) is False
    
        # Check the behavior when a path is provided
        tmp_dir = TemporaryDirectory()
        with download_path(tmp_dir.name) as path:
            assert os.path.isdir(path) is True
>       assert os.path.isdir(path) is False
E       AssertionError: assert True is False
E        +  where True = <function isdir at 0x14f2671f63b0>('/tmp/tmpfor_zw3k')
E        +    where <function isdir at 0x14f2671f63b0> = <module 'posixpath' from '/azureml-envs/azureml_f7453134e7e1597f40f148b7ddc0ba6e/lib/python3.10/posixpath.py'>.isdir
E        +      where <module 'posixpath' from '/azureml-envs/azureml_f7453134e7e1597f40f148b7ddc0ba6e/lib/python3.10/posixpath.py'> = os.path

tests/unit/recommenders/datasets/test_download_utils.py:79: AssertionError
______________________________ test_merge_rating _______________________________

rating_true =     userID  itemID  rating
0        1       3       3
1        2       1       5
2        2       4       5
3        2... 12       3
14       3      13       2
15       3      14       1
16       1       1       5
17       1       2       4
rating_pred =     userID  itemID  prediction  rating
0        1      12          12       3
1        2      10          14       5
2... 2
15       3      14           5       1
16       1       3          14       5
17       1      10          13       4

    def test_merge_rating(rating_true, rating_pred):
        y_true, y_pred = merge_rating_true_pred(
            rating_true,
            rating_pred,
            col_user=DEFAULT_USER_COL,
            col_item=DEFAULT_ITEM_COL,
            col_rating=DEFAULT_RATING_COL,
            col_prediction=DEFAULT_PREDICTION_COL,
        )
        target_y_true = np.array([3, 3, 5, 5, 3, 3, 2, 1])
        target_y_pred = np.array([14, 12, 7, 8, 13, 6, 11, 5])
    
        assert y_true.shape == y_pred.shape
>       assert np.all(y_true == target_y_true) is True
E       assert True is True
E        +  where True = <function all at 0x14f262569a20>(0    3\n1    3..., dtype: int64 == array([3, 3, ..., 3, 3, 2, 1])
E        +    where <function all at 0x14f262569a20> = np.all
E           
E           Use -v to get more diff)

tests/unit/recommenders/evaluation/test_python_evaluation.py:120: AssertionError