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

make globus and paramiko optional dependencies #3156

Closed
wants to merge 2 commits into from
Closed

make globus and paramiko optional dependencies #3156

wants to merge 2 commits into from

Conversation

azharcodeit
Copy link

Description

Place globus and paramiko into optional dependencies list, due to rare usage and paramiko's incorrect binary installs of its unix .so file.

Changed Behaviour

This PR doesn't affect any user workflow or functionality.

Fixes

Fixes #2218

Type of change

Choose which options apply, and delete the ones which do not apply.

  • Code maintenance/cleanup

Copy link
Collaborator

@benclifford benclifford left a comment

Choose a reason for hiding this comment

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

Hi. Adding ssh and globus install targets like this is the right way to go.

paramiko and globus_sdk are still listed in requirements.txt, which means they will still be installed automatically, even if a user does not request pip install parsl[ssh] or pip install parsk[globus].

So you should at least remove those lines from requirements.txt to balance out the new install targets that you added in this PR.

I think if you do that and push up to GitHub, you will see some more test failures (because some Parsl code expects ssh and globus to be installed) - that's ok, we should expect these failures.

The next thing to do would be to work on fixing each failure, which I will explain when we see them.

If there aren't any test failures, that is suspicious - because I would expect something to fail here when we stop installing these dependencies.

@azharcodeit
Copy link
Author

azharcodeit commented Mar 6, 2024

Thank you for your feedback, @benclifford! I made changes according to your comment about removing dependencies from requirements.txt as well.

As you said some checks have failed indeed, can you please have a look, so that I proceed with fixes.
So far, I came across with the following error:

parsl/channels/ssh/ssh.py:5: in <module>
    import paramiko
E   ModuleNotFoundError: No module named 'paramiko'
make: *** [Makefile:52: local_thread_test] Error 4
Error: Process completed with exit code 2.

@benclifford
Copy link
Collaborator

It looks like you figured out the approach other parts of the Parsl codebase use to work with optional modules.

I can see that the tests are failing at the moment, with this error:

ImportError while loading conftest '/home/runner/work/parsl/parsl/parsl/tests/conftest.py'.
parsl/__init__.py:22: in <module>
    from parsl.app.app import bash_app, join_app, python_app
parsl/app/app.py:12: in <module>
    from parsl.dataflow.dflow import DataFlowKernel
parsl/dataflow/dflow.py:25: in <module>
    from parsl.channels import Channel
parsl/channels/__init__.py:2: in <module>
    from parsl.channels.ssh.ssh import SSHChannel
parsl/channels/ssh/ssh.py:27: in <module>
    raise OptionalModuleMissing(['paramiko'], "OAuthSSHChannel requires paramiko module.")
E   parsl.errors.OptionalModuleMissing: The functionality requested requires optional modules ['paramiko'] which could not be imported, because: OAuthSSHChannel requires paramiko module.

That's happening because this block of code in ssh.py raises an exception at import time if the optional module is missing - but we need this file to be importable even if the optional module is missing:

if _paramiko_enabled:
    class NoAuthSSHClient(paramiko.SSHClient):
        def _auth(self, username, *args):
            self._transport.auth_none(username)
            return
else:
    raise OptionalModuleMissing(['paramiko'], "OAuthSSHChannel requires paramiko module.")

Perhaps you could try removing that else raise section - so that if the optional module is available, the NoAuthSSHClient will be defined, and if it is not, then it will silently be skipped.

Then, the only use of NoAuthSSHClient is protected by a different OptionalModuleMissing, that you have added at the start of SSHChannel.__init__

@azharcodeit azharcodeit marked this pull request as draft March 25, 2024 01:35
@azharcodeit azharcodeit closed this by deleting the head repository Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make globus and paramiko optional dependencies
2 participants