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

Create profile loading mechanism #154

Merged
merged 5 commits into from Sep 30, 2015
Merged

Conversation

@Bachmann1234
Copy link
Contributor

@Bachmann1234 Bachmann1234 commented Sep 20, 2015

Allows users to register settings profiles and load them
in as needed. Allowing for different defaults for different
environments

Is this kind of what you were thinking for the profile setup?

This code should enable the following pattern

Settings.register_profile('dev', max_examples=5, max_shrinks=10, max_iterations=100)
Settings.register_profile('ci', max_examples=1000, max_shrinks=1000, max_iterations=2000)

Settings.load_profile(os.getenv('HYPOTHESIS_PROFILE', 'default'))

If you think im close ill iterate with you as needed. If we get to something you like let me know and ill write up the docs.

@kxepal
Copy link
Member

@kxepal kxepal commented Sep 20, 2015

Nice feature, but I think you came from a wrong side.

Assume you have multiple environments: dev, ci, stage, prod. For each you already have some configuration file. Otherwise there is no need in such division. And in these config files you may define hypothesis settings whatever you need and just let read them somewhere in tests/__init__.py to apply them before tests will get run. So you eventually don't need yet another configuration-per-environment thing.

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 20, 2015

@kxepal thats a good point. I guess I could just create the settings object on launch.

The history here I wanted to set max_examples with an environment variable #153 but then @DRMacIver pointed me to the idea of profiles he had http://www.drmaciver.com/2015/08/a-vague-roadmap-for-hypothesis-2-0/

But yeah, I guess since you can just define the settings when your test file gets loaded there may not be much of a need here. Maybe @DRMacIver had something more involved in mind for the profiles feature

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 21, 2015

So I still think this is a good feature, and I didn't really have anything much more involved in mind. :-)

It's not a very complicated feature, but having it makes peoples' lives easier and is useful for when you want to try something locally but faster.

In particular I think doing this in your test config basically requires multiple people to reinvent something that looks more or less like this. Additionally, I'd really like Hypothesis to ship with a bunch of built in profiles (I'm happy to write these @Bachmann1234, but you should feel to add any you like).

Anyway, thanks for doing this @Bachmann1234! This basically looks pretty good. I'll add some specific comments inline.

Loading

@@ -257,8 +259,41 @@ def __exit__(self, *args, **kwargs):
default_context_manager = self.defaults_stack().pop()
return default_context_manager.__exit__(*args, **kwargs)

default = DefaultSettings()
@staticmethod
def register_profile(name, **kwargs):
Copy link
Member

@DRMacIver DRMacIver Sep 21, 2015

Choose a reason for hiding this comment

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

So in my head you would use explicit Settings objects here rather than kwargs, but I could be persuaded that kwargs are the way to go, or to support both modes. Is there a reason you wanted to do this as kwargs?

Loading

Copy link
Contributor Author

@Bachmann1234 Bachmann1234 Sep 22, 2015

Choose a reason for hiding this comment

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

I was mostly trying to be more concise. But using the specific object would be more consistent.

Loading

Copy link
Contributor Author

@Bachmann1234 Bachmann1234 Sep 22, 2015

Choose a reason for hiding this comment

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

Not to mention using the actual object is more in line with the whole Zen thing Explicit is better than implicit. :-P

Loading

@The-Compiler
Copy link
Contributor

@The-Compiler The-Compiler commented Sep 21, 2015

What about having HYPOTHESIS_PROFILE interpreted by default?

Loading

except KeyError:
raise ValueError("Profile '{}' has not been registered", name)
for key, value in profile.items():
setattr(Settings.default, key, value)
Copy link
Member

@DRMacIver DRMacIver Sep 21, 2015

Choose a reason for hiding this comment

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

Assigning individual settings seems like an odd choice. In particular Settings.default is assignable and it seems worth taking advantage of that.

It would be nice for example to support things that had done:

@given(blah, settings=Settings.default)
def test_stuff(blah):
    ...

not have Settings changed underneath them.

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 21, 2015

I'm also strongly in favour of @The-Compiler's suggestion of having HYPOTHESIS_PROFILE interpreted by default.

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 21, 2015

Sound good. I read over the comments. Ill address them sometime early this week depending on my schedule.

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 22, 2015

So I addressed some of the comments but hit a snag.

I tried setting the default settings object rather then set things by hand.

@staticmethod
    def load_profile(name):
        """
        Loads in the settings defined in the profile provided
        If the profile does not exist an AttributeError will
        be thrown. Any setting not defined in the profile
        will be the library defined default for that setting
        """
        Settings.default = Settings.get_profile(name)

But this seems to break the context manager concept.

This test I added fails

def test_loading_profile_keeps_expected_behaviour():
    Settings.register_profile("ci", Settings(max_examples=10000))
    Settings.load_profile("ci")
    assert Settings().max_examples == 10000
    with Settings(max_examples=5):
        assert Settings().max_examples == 5
    assert Settings().max_examples == 10000

Any thoughts? What am I missing here?

I added the loading in a profile with an environment var btw. Testing it was a bit hacky though... See test_load_on_environment I could simply track the currently loaded profile... but I figured for now avoid the extra state.

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 22, 2015

Oh. I was thinking of adding a log when a profile is loaded in

"Profile 'ci' loaded"

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 23, 2015

How is the test with the context manager failing? It looks like it should work for me.

One thing that I think might be worth doing is making assigning Settings.default an error when you're inside a Settings context manager. It's... arguably a breaking change to the API, but it's so underdocumented that I'm not sure I could argue that the ability to do so is actually public, and I'm almost certain nobody is doing it and completely certain that it's not doing what they expect if they are.

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 23, 2015

Although I guess it's important that the ability to nest context managers works even if flat out assigning Settings.default doesn't.

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 23, 2015

If you do add a log I have a silly edge case question that I'm not sure what the best answer to is: What verbosity level do you use (I'd guess debug) and do you use the verbosity level of the new or the old settings value?

Loading

try:
return Settings._profiles[name]
except KeyError:
raise ValueError("Profile '{}' has not been registered".format(name))
Copy link
Member

@DRMacIver DRMacIver Sep 23, 2015

Choose a reason for hiding this comment

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

I would use InvalidArgument from hypothesis.errors here.

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 23, 2015

I guess on further thought there's no reason assignment inside a context manager can't just be a DeprecationWarning for now and keep the old and very broken behaviour in case anyone is actually using it. Has similar questions about which strict mode applies, but I think it makes sense for it to be the current one.

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 23, 2015

Extension to the feature that occurs to me that I'm going to record here but you should feel 100% free not to implement: It would be nice if the pytest plugin exposed a command line argument for setting the profile.

Loading

@Bachmann1234 Bachmann1234 force-pushed the profiles branch 2 times, most recently from f37e3e2 to 0d576df Sep 24, 2015
@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 24, 2015

So here is how the test fails

/Users/bachmann/Code/hypothesis/tests/cover/test_settings.py:184: in test_loading_profile_keeps_expected_behaviour
    assert Settings().max_examples == 5
E   assert 10000 == 5
E    +  where 10000 = Settings(a_setting_just_for_these_tests=3, a_setting_with_limited_options=3, average_list_length=25.0, database_file='..., max_iterations=1000, max_shrinks=500, min_satisfying_examples=5, strict=True, timeout=-1, verbosity=Verbosity.normal).max_examples
E    +    where Settings(a_setting_just_for_these_tests=3, a_setting_with_limited_options=3, average_list_length=25.0, database_file='..., max_iterations=1000, max_shrinks=500, min_satisfying_examples=5, strict=True, timeout=-1, verbosity=Verbosity.normal) = Settings()

Basically, the option declared in the with statement is not being respected

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 24, 2015

After much digging I got the context manager working as expected. Basically I needed to set Settings.default_variable.data.value rather than Settings.default

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 24, 2015

So as an update here is my TODO on this PR

  1. See if my pytest option actually works as expected (looks like pytest stuff is tested by hand so ill just run some tests and put the output in here)
  2. Squash Bachmann1234@6ac683f into Bachmann1234@0d576df Leaving them both here (for now) separately so you can compare the diff if you're inclined.
  3. Write docs for this feature

As for the log I basically scrapped it after not being able to decide the best resolution to the verbosity question. To Log based on the previous or the new setting.

Anything else im forgetting here?

Loading

@The-Compiler
Copy link
Contributor

@The-Compiler The-Compiler commented Sep 24, 2015

(looks like pytest stuff is tested by hand so ill just run some tests and put the output in here)

Judging from the amount of tests hypothesis has, I don't think anything is tested by hand - automate ALL the things! 😉

See tests/pytest.

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 24, 2015

@The-Compiler AH! Thanks, ill update that either tomorrow or friday

Loading

# Monkeypatch the environment
monkeypatch.setenv("HYPOTHESIS_PROFILE", "ci")
with pytest.raises(hypothesis.errors.InvalidArgument):
# Reload. Non existent profile loaded so ValueError thrown
Copy link
Member

@DRMacIver DRMacIver Sep 24, 2015

Choose a reason for hiding this comment

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

This comment is now wrong because it's an InvalidArgument thrown instead.

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 24, 2015

Additional TODO items:

  1. I've added a few trivial comments inline
  2. The build is currently broken due to a mix of linting (you can fix this just by running tox -e lint and committing the changes) and possibly more serious issues (Settings has no attribute 'database'), which needs fixing.
  3. You should definitely add yourself to the contributors list. :-)

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 24, 2015

BTW I've seen the 'Settings has no attribute 'database'' issue elsewhere and I'm pretty sure what's causing it is that you've got an AttributeError triggered inside the 'database' property. This causes Python to invoke the getattr method (because Python attribute resolution is hilarious). The easiest way to debug what's going on is to call Settings.default.database directly and see what happens.

Loading

@@ -198,6 +201,10 @@ def define_setting(cls, name, description, default, options=None):
all_settings[name] = Setting(
name, description.strip(), default, options)
setattr(cls, name, SettingsProperty(name))
if cls.default:
setattr(cls.default, name, default)
for profile in cls._profiles.values():
Copy link
Member

@DRMacIver DRMacIver Sep 29, 2015

Choose a reason for hiding this comment

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

This doesn't look correct to me. What happens if I do:

x = Settings()
define_setting('blah', default=3, ...)
Settings.register_profile('hi', Settings(blah=2))
assert x.blah == 3
assert Settings.get_profile('hi') == 3

Loading

Copy link
Contributor Author

@Bachmann1234 Bachmann1234 Sep 30, 2015

Choose a reason for hiding this comment

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

Ok so I added a test

def test_define_setting_then_loading_profile():
    x = Settings()
    Settings.define_setting(
        u'fun_times',
        default=3, description=u'Something something spoon',
        options=(1, 2, 3, 4),
    )
    Settings.register_profile('hi', Settings(fun_times=2))
    assert x.fun_times == 3
    assert Settings.get_profile('hi').fun_times == 2

So I make a settings object x

I define a new setting

x gets the default value while any new profiles get the value I set. Seems right to me. Though this PR is the lat thing I do before bed so I would not be surprised if i'm just being dense

Loading

Copy link
Member

@DRMacIver DRMacIver Sep 30, 2015

Choose a reason for hiding this comment

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

I'm very confused what's going on here, but this PR is the first thing I do when drinking my coffee so I also would not be surprised if I were being dense. :-)

Why is the setattr(profile, name, default) not clobbering this? There must be an attribute lookup happening before the assignment I guess, but relying on that seems very suspect.

OTOH with the test there I don't mind too much. The settings system is due for a significant rework and I'm happy to get this merged and do the internals cleanup myself.

Loading

Copy link
Contributor Author

@Bachmann1234 Bachmann1234 Sep 30, 2015

Choose a reason for hiding this comment

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

honestly im not 100% sure. You mentioned attribute lookup. When I register/load profiles I perform a copy so that may do it

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 29, 2015

The current set of failing tests is almost entirely novel, but I don't see how it can be your fault. Don't worry about them for now, I'll look into it. Yet another battle in the endless war of Hypothesis build stability. Sorry.

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 30, 2015

Added to extra packages
screen shot 2015-09-29 at 11 35 17 pm

Added to settings profiles
screen shot 2015-09-29 at 11 35 01 pm

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 30, 2015

Sorry, the ascii failures are a problem that's worked around in master (It's a bug in py.test 2.8.1).

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 30, 2015

That is, if you rebase this off master they should go away.

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 30, 2015

I think once you've rebased off master to fix the pytest issues this LGTM. Are you aware of anything else you need to work on before merging?

Loading

Allows users to register settings profiles and load them
in as needed. Allowing for different defaults for different
environments
Any profile loaded in by pytest needs to be defined in either
a conftest or a file loaded in by a conftest as the profile loading
happens after the conftests are loaded but before any test code
is executed
@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 30, 2015

@DRMacIver rebased. Lets see what the build says

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 30, 2015

Timeout? That's new

Loading

@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 30, 2015

A timeout is usually just Travis being under load. I've rebooted the job that failed and will rerun it, but I'm pretty sure it will pass and then I will merge. \o/

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 30, 2015

Woot!

Loading

DRMacIver added a commit that referenced this issue Sep 30, 2015
Create profile loading mechanism
@DRMacIver DRMacIver merged commit aeaad38 into HypothesisWorks:master Sep 30, 2015
1 of 2 checks passed
Loading
@DRMacIver
Copy link
Member

@DRMacIver DRMacIver commented Sep 30, 2015

:shipit: Thanks so much for all your work on this. Sorry it took so long.

Loading

@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 30, 2015

No problem!

Loading

@Bachmann1234 Bachmann1234 deleted the profiles branch Sep 30, 2015
@Bachmann1234
Copy link
Contributor Author

@Bachmann1234 Bachmann1234 commented Sep 30, 2015

@DRMacIver if any issues come up around this dont be afraid to reach out

Loading

>>> Settings.register_profile("debug", Settings(max_examples=10, verbosity=Verbosity.verbose))
>>> Settings.load_profile(os.getenv(u'HYPOTHESIS_PROFILE', 'default'))
If you are using the hypothesis pytest plugin. If the profile was registered
Copy link
Contributor

@The-Compiler The-Compiler Sep 30, 2015

Choose a reason for hiding this comment

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

These two sentences are confusing me - shouldn't this be something like "If you are using the hypothesis pytest plugin and the profile ..."?

Loading

Copy link
Contributor Author

@Bachmann1234 Bachmann1234 Sep 30, 2015

Choose a reason for hiding this comment

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

Sure, I can edit that tonight. and open a quick PR

Loading

@The-Compiler
Copy link
Contributor

@The-Compiler The-Compiler commented Sep 30, 2015

Sorry I didn't look at this before the merge - but the two things above (especially the second one) probably need some amending 😉

Loading

jwg4 added a commit to jwg4/hypothesis that referenced this issue May 26, 2018
The url pattern https://github.com/HypothesisWorks/hypothesis/pulls/154 should be HypothesisWorks#154 . The former goes to a pull request search page and queries for PRs with an author named '154'. See for example https://hypothesis.readthedocs.io/en/latest/development.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants