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

add warning for when UCX_MEM_MMAP_HOOK_MODE is set #626

Open
wants to merge 2 commits into
base: branch-0.16
Choose a base branch
from

Conversation

quasiben
Copy link
Member

Related to #616

cc @pentschev

@quasiben quasiben requested a review from a team as a code owner September 30, 2020 17:14
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

@quasiben I added a few suggestions there.

ucp/__init__.py Outdated Show resolved Hide resolved
ucp/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @quasiben !

@jakirkham
Copy link
Member

Am seeing the following failure on CI.

___________________________ test_mem_mmap_hook_warn ____________________________

caplog = <_pytest.logging.LogCaptureFixture object at 0x7f138eb82850>

    def test_mem_mmap_hook_warn(caplog):
        """
        Test warning for UCX_MEM_MMAP_HOOK_MODE
        """
        import logging
    
        os.environ["UCX_MEM_MMAP_HOOK_MODE"] = "none"
    
        # ucp.init will only print INFO LINES
        with caplog.at_level(logging.INFO):
            import ucp
    
>           ucp.init()

tests/test_mmap_warn.py:16: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

options = {}, env_takes_precedence = False, blocking_progress_mode = None

    def init(options={}, env_takes_precedence=False, blocking_progress_mode=None):
        """Initiate UCX.
    
        Usually this is done automatically at the first API call
        but this function makes it possible to set UCX options programmable.
        Alternatively, UCX options can be specified through environment variables.
    
        Parameters
        ----------
        options: dict, optional
            UCX options send to the underlying UCX library
        env_takes_precedence: bool, optional
            Whether environment variables takes precedence over the `options`
            specified here.
        blocking_progress_mode: bool, optional
            If None, blocking UCX progress mode is used unless the environment variable
            `UCXPY_NON_BLOCKING_MODE` is defined.
            Otherwise, if True blocking mode is used and if False non-blocking mode is used.
        """
        global _ctx
        if _ctx is not None:
            raise RuntimeError(
>               "UCX is already initiated. Call reset() and init() "
                "in order to re-initate UCX with new options."
            )
E           RuntimeError: UCX is already initiated. Call reset() and init() in order to re-initate UCX with new options.

ucp/core.py:755: RuntimeError

@pentschev
Copy link
Member

Hmmm, yeah testing for UCX variables can be really tricky. The "solution" for that would be calling ucp.reset and then ucp.init. However, given pytest's behavior of not creating a fresh process for each new test, this could mean that we would be modifying behavior of tests that happen to run after this one. Due to that, I would prefer if we just don't add a test for this, given it's just a warning the test isn't as critical as for other functionality. Thoughts on this @quasiben @jakirkham ?

@jakirkham
Copy link
Member

Yeah that's ok with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants