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

ENH: Delay etelemetry for non-interactive sessions, report bad versions #3049

Merged
merged 19 commits into from
Oct 3, 2019

Conversation

satra
Copy link
Member

@satra satra commented Sep 24, 2019

Summary

Improves version checking

this will require the server to support the .et file in a project directory

List of changes proposed in this PR (pull-request)

  • check for return from server and do not print anything if there is no change in version
  • checking is it's own function and raises exception if asked for bad versions

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

ENH: Run memoized check_version at REPL import, Node/Workflow/Interface init
@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #3049 into master will increase coverage by <.01%.
The diff coverage is 74.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3049      +/-   ##
==========================================
+ Coverage   67.72%   67.72%   +<.01%     
==========================================
  Files         344      344              
  Lines       44087    44108      +21     
  Branches     5554     5562       +8     
==========================================
+ Hits        29856    29872      +16     
- Misses      13465    13470       +5     
  Partials      766      766
Flag Coverage Δ
#smoketests 50.31% <74.07%> (+0.02%) ⬆️
#unittests 65.14% <14.81%> (-0.04%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/core.py 87.11% <100%> (+0.12%) ⬆️
nipype/__init__.py 65.07% <68.18%> (+1.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 085767c...c7e3934. Read the comment docs.

* upstream/master:
  FIX: Handle missing paths better pre-3.6
  RF: Provide functions to augment old Path.mkdir, Path.resolve methods
def sys_based_cache(condition):
def decorator(func):
if condition:
return func
Copy link
Member

Choose a reason for hiding this comment

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

This means that if somebody's using Python 2, we're going to ping on every version. Since we're only worrying about one function with one boolean argument for a couple more releases, we can do a very quick-and-dirty memoization:

class _memoize(object):
    def __init__(self, func):
        self.cache = {}
        self.func = func
    def __call__(self, *args, **kwargs):
        argtuple = (args,) + tuple(kwargs.items())
        if argtuple not in self.cache:
            self.cache[argtuple] = self.func(*args, **kwargs)
        return self.cache[argtuple]

Another option is to manage a global state ourselves

etelemetry_results = {}

def check_latest_version(raise_exception=False):
    if raise_exception not in etelemetry_results:
        ...  # Current function
        etelemetry_results[raise_exception] = latest
    return etelemetry_results[raise_exception]

Or we could use something like memoization, which is surely a big hammer, but won't add code that will be most sensibly removed in two months.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, thinko. That should be "ping on every interface/node/workflow instantiation", in case your brain didn't make the leap. I think it's a pretty major drawback, although I don't know how many Python 2 users we still have.

@satra
Copy link
Member Author

satra commented Sep 30, 2019

the failing test is #3046

we should merge this in before a release - and also fix up the bad_versions keys

@satra satra requested a review from effigies October 2, 2019 19:20
nipype/__init__.py Outdated Show resolved Hide resolved
nipype/__init__.py Outdated Show resolved Hide resolved
nipype/interfaces/base/core.py Outdated Show resolved Hide resolved
nipype/__init__.py Outdated Show resolved Hide resolved
* upstream/master:
  tst: use pytest fixture
* upstream/master:
  TEST: Mark test_dcm2niix_dti XFAIL
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM. Is this ready to go?

@effigies effigies changed the title improve etelemetry operations ENH: Delay etelemetry for non-interactive sessions, report bad versions Oct 3, 2019
.et
Comment on lines +1 to +2
{ "bad_versions" : [ "1.2.1",
"1.2.3"]
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be checked and updated before release.

Copy link
Member

Choose a reason for hiding this comment

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

I think these are legitimately buggy. We can really only plausibly check back to 1.3.0, since this functionality will be missing before that. Is it worth looking for critical bugs in earlier release logs?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was more about whether even these two were correct. from a usage perspective, nothing will change till the server starts reading these .et files and returning the info. i'll update the server soon.

@satra satra merged commit 7d310dd into nipy:master Oct 3, 2019
@effigies effigies added this to the 1.3.0 milestone Oct 3, 2019
@satra
Copy link
Member Author

satra commented Nov 19, 2019

@mgxd @effigies - fyi

$ git co 88d479b
Note: checking out '88d479b'.

HEAD is now at 88d479b69 Merge pull request #3093 from nipy/rel/1.3.0

$ ipython
Python 3.7.5 (default, Oct 25 2019, 10:52:18) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.9.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import nipype                                                                                                                      
191119-17:14:19,512 nipype.utils INFO:
	 Running nipype version 1.3.0 (latest: 1.3.1)
191119-17:14:19,512 nipype.utils CRITICAL:
	 You are using a version of Nipype with a critical bug. Please use a different version.

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.

2 participants