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.

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

Fix L0_backend_python iGPU PyTorch installation #7231

Merged
merged 2 commits into from
May 16, 2024
Merged

Conversation

kthui
Copy link
Contributor

@kthui kthui commented May 16, 2024

Before the fix, the test was comparing

+ [[ 1 == \0 ]]

for

[[ "$TEST_JETSON" == "0" ]]

After the fix, the comparison is restored to

+ '[' 1 == 0 ']'

Also fix a comparison logic error in using || and &&, before

if [[ "$TEST_JETSON" == "0" ]] || [[ ${TEST_WINDOWS} == 0 ]]; then
    # Run Linux setup
else
    # Run iGPU or Windows setup
fi

after

if [ "$TEST_JETSON" == "0" ] && [[ ${TEST_WINDOWS} == 0 ]]; then
...

@kthui
Copy link
Contributor Author

kthui commented May 16, 2024

@fpetrini15 I couldn't get the Windows build to work. Do you see any concern on Windows test using one square bracket comparison instead of two? i.e. [ "$TEST_JETSON" == "0" ] vs [[ "$TEST_JETSON" == "0" ]]

(The Jetson comparison is comparing a string, while the Windows comparison is comparing an integer)

@kthui kthui marked this pull request as ready for review May 16, 2024 16:56
@kthui kthui changed the title Fix failed flag comparison on Jetson Fix iGPU Python test flag comparison May 16, 2024
@kthui kthui changed the title Fix iGPU Python test flag comparison Fix L0_backend_python iGPU PyTorch installation May 16, 2024
@fpetrini15
Copy link
Contributor

fpetrini15 commented May 16, 2024

@kthui good catch on the incorrect logical operator!

With respect to the brackets, I believe the guidance is to use the double bracket notation whenever possible. For example, if we have the following script where $TEST_JETSON was left unquoted:

#!/bin/bash
TEST_JETSON=
if [[ $TEST_JETSON == "0" ]]; then
        echo "FLAG equals 0"
else
        echo "FLAG does not equal 0"
fi

Will correctly evaluate to the else condition with no error.

With single brackets:

#!/bin/bash
TEST_JETSON=
if [ $TEST_JETSON == "0" ]; then
        echo "FLAG equals 0"
else
        echo "FLAG does not equal 0"
fi

We will get the error: test.sh: line 3: [: ==: unary operator expected

I know this is assuming layers of mistakes made by a developer, but I believe the double-bracket is still best-practice.

Additionally, though bash -x seems to interpret "0" / "1" as \0 and \1 when it prints to console, the comparison appears to be sound and will correctly evaluate.

All that said, as it relates to your original question, I see no issue, though it would probably be more correct to use the -eq operator if we expect an integer. I tested the cases locally with no issue, but I will follow-up if I see a failure.

@kthui
Copy link
Contributor Author

kthui commented May 16, 2024

@fpetrini15 thanks for checking it on Windows! I have no issue moving everything to double bracket, as long as it doesn't backfire on us.

On your single bracket case, if you double quote the "$TEST_JETSON" variable, it works on my local machine:

#!/bin/bash
TEST_JETSON=
if [ "$TEST_JETSON" == "0" ]; then
        echo "FLAG equals 0"
else
        echo "FLAG does not equal 0"
fi

The double quote variant is what is used on the test script and it is already passing on the CI. I don't thinks this will become an issue on the test on x86 in the future.

Do you think it is ok to keep the single bracket for the "$TEST_JETSON" variable for now? It's because we want to pick this fix into 24.05 release and this variant is already passing on the CI. If we switch to the double bracket variant, we will have to run the CI again and likely have another half day delay on this.

@fpetrini15
Copy link
Contributor

fpetrini15 commented May 16, 2024

@kthui you are correct. If you add quotes to the variable there is no issue. In general, I understand there is no issue now, but I believe it is an additional safeguard for the future. Just something I wanted to start a discussion about.

Either way, not important enough to hold up this PR/release. You can merge when ready! 🚀

@tanmayv25
Copy link
Contributor

Thanks @kthui for the quick fix!

@kthui kthui merged commit 23a6c21 into main May 16, 2024
3 checks passed
@kthui kthui deleted the jacky-fix-igpu-test-flag branch May 16, 2024 20:02
kthui added a commit that referenced this pull request May 16, 2024
* Fix TEST_JETSON double square bracket issue

* Use and instead of or
mc-nv pushed a commit that referenced this pull request May 16, 2024
* Fix TEST_JETSON double square bracket issue

* Use and instead of or
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants