In code_utils.py, handling NotADirectoryError as well in get_powershell_command

[abhaymathur21]

Why are these changes needed?

On MacOS for example, the windows powershell directory does not even exist and the python subprocess looks for the powershell directory in the path and throws a NotADirectoryError. In windows, the powershell directory exists so that error is not thrown but the file name may be different (powershell.exe or pwsh.exe) so a FileNotFoundError is thrown.

We must be able to deal with both these scenarios, hence these changes are necessary.

Related issue number

Closes #1947

Checks

• I've included any doc changes needed for https://microsoft.github.io/autogen/. See https://microsoft.github.io/autogen/docs/Contribute#documentation to build and test documentation locally.
• I've added tests (if relevant) corresponding to the changes introduced in this PR.
• I've made sure all auto checks have passed.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.76%. Comparing base (b0a8e6e) to head (32f580a).

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1948       +/-   ##
===========================================
+ Coverage   37.33%   47.76%   +10.42%     
===========================================
  Files          64       64               
  Lines        6913     6913               
  Branches     1519     1650      +131     
===========================================
+ Hits         2581     3302      +721     
+ Misses       4109     3336      -773     
- Partials      223      275       +52     

Flag	Coverage Δ
unittests	47.63% <100.00%> (+10.29%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sonichi]

@lestan could you test this PR in your environment? Thanks.

[lestan]

Hi @abhaymathur21

Thanks for looking into this and sharing your explanation. By that logic, shouldn't the function check for WIN32 before going into the main if statement and return None for any other platform? Seems unnecessary to execute the logic, trigger exceptions and handle them twice when we know that PowerShell is only available on Windows.

Thanks

Les

[abhaymathur21]

Hi @abhaymathur21

Thanks for looking into this and sharing your explanation. By that logic, shouldn't the function check for WIN32 before going into the main if statement and return None for any other platform? Seems unnecessary to execute the logic, trigger exceptions and handle them twice when we know that PowerShell is only available on Windows.

Thanks

Les

That is a good suggestion but as far as I know it is possible to install and use powershell in various linux distributions as well and is a commonly followed practice so that's why it might be better to check the win32 condition later as I have given in the code

[lestan]

Hi @abhaymathur21 and @sonichi,

I checked out the pr and built and ran it successfully. To test, I ran the test/twoagent.py and the test_chats test from test/agentchat/test_chats.py. Both tests passed.

Confirming the changes are good.

Thank you for the quick response to this!

Let me know if you need anything more from me to merge this pr. Looking forward to getting the fix into the next release!

[abhaymathur21]

Hi @abhaymathur21 and @sonichi,

I checked out the pr and built and ran it successfully. To test, I ran the test/twoagent.py and the test_chats test from test/agentchat/test_chats.py. Both tests passed.

Confirming the changes are good.

Thank you for the quick response to this!

Let me know if you need anything more from me to merge this pr. Looking forward to getting the fix into the next release!

Happy to help! Thank you for pointing this issue out.

[lestan]

Confirmed this is working as expected in release 0.2.18! ⭐