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

profile setup fails due to invalid timezone #3563

Closed
ltalirz opened this issue Nov 21, 2019 · 9 comments · Fixed by #3576
Closed

profile setup fails due to invalid timezone #3563

ltalirz opened this issue Nov 21, 2019 · 9 comments · Fixed by #3576

Comments

@ltalirz
Copy link
Member

ltalirz commented Nov 21, 2019

On the Midway cluster at the University of Chicago, it turns out that this code

from tzlocal import get_localzone
return get_localzone()

returns a timezone called local which when passed here

TIME_ZONE = get_current_timezone().zone

gives rise to an error

ValueError: Incorrect timezone setting: local

when the django migrations are running.

We should look into why django and tzlocal seem to have a different idea of which timezones are allowed, and whether we can provide a fallback (or a suggestion to fix it) so that profile setup does not simply fail.

Note: export TIME_ZONE='America/Chicago' as suggested here did not help

@ramirezfranciscof
Copy link
Member

Haha, it seems we need to add a travis battery of tests that proxies to the US before running.

Also, interestingly enough, we have a tag for sqlalchemy but not one for django. Should I create it?

@ltalirz
Copy link
Member Author

ltalirz commented Nov 21, 2019

Also, interestingly enough, we have a tag for sqlalchemy but not one for django. Should I create it?

Please go ahead!

@greschd
Copy link
Member

greschd commented Nov 21, 2019

Note: export TIME_ZONE='America/Chicago' as suggested here did not help

Isn't what this link suggests to set TIME_ZONE = 'America/Chicago' in the django settings.py?

@greschd
Copy link
Member

greschd commented Nov 21, 2019

According to this paragraph tzinfo is working as intended in this case, but the timezone is not configured on that system.

Since local is not in the list of time zones as linked by Django I think the libraries are all working as expected. We could check on our side whether get_current_timezone().zone is "local" and set it to "UTC" as a sensible fallback. This would probably mean that time stamps end up being wrong, though. I'm not sure if setting USE_TZ = False in that case would be possible (i.e., not guessing the timezone at all), or if this would break other things that assume there is a TZ set.

@ltalirz
Copy link
Member Author

ltalirz commented Nov 21, 2019

Isn't what this link suggests to set TIME_ZONE = 'America/Chicago' in the django settings.py?

Ah, right ;-) Of course, that's also what we did to work around this.

According to this paragraph tzinfo is working as intended in this case, but the timezone is not configured on that system. Since local is not in the list of time zones as linked by Django I think the libraries are all working as expected.

Thanks a lot for taking the time to look into this!

We could check on our side whether get_current_timezone().zone is "local" and set it to "UTC" as a sensible fallback. This would probably mean that time stamps end up being wrong, though.

Sounds reasonable to me - perhaps together with printing a warning?
Definitely preferable to verdi setup simply failing.
The alternative would be if one can print an error message that provides instructions on how to tell tzlocal the right timezone?

I'm not sure if setting USE_TZ = False in that case would be possible (i.e., not guessing the timezone at all), or if this would break other things that assume there is a TZ set.

Ok, we should check.

@greschd greschd closed this as completed Nov 22, 2019
@greschd greschd reopened this Nov 22, 2019
@greschd
Copy link
Member

greschd commented Nov 22, 2019

Sounds reasonable to me - perhaps together with printing a warning?
Definitely preferable to verdi setup simply failing.

Yeah, if it's in interactive mode we can just ask something like "Time stamps will be inaccurate. Are you sure you want to continue?"

The alternative would be if one can print an error message that provides instructions on how to tell tzlocal the right timezone?

It seems running tzselect is how to choose a time zone from the command line, but I'm not sure if that is guaranteed to be available. Unless having the wrong timezone messes things up a lot somewhere down the line I'd still vote for having a fallback.

I'm not sure if setting USE_TZ = False in that case would be possible (i.e., not guessing the timezone at all), or if this would break other things that assume there is a TZ set.

Ok, we should check.

From the comment (see below) I'd say USE_TZ = False "probably breaks things", but I'm not sure. In any case it's not much better than just setting it to UTC. It only makes it explicit that the time zone cannot be trusted.

# If you set this to False, Django will not use timezone-aware datetimes.

@ltalirz
Copy link
Member Author

ltalirz commented Nov 26, 2019

Looking at the implementation of tzlocal, it turns out that local actually does not mean that the time zone is unknown.
It just means that the "zoneinfo name" is unknown.

As discussed with @sphuber , one solution is to actually delete this setting

TIME_ZONE = get_current_timezone().zone

since it only configures how django displays datetime objects; a functionality that we don't use in AiiDA.

Mocking the local timezone using

def get_current_timezone():
    import pytz
    with open('/etc/localtime', 'rb') as handle:
        tz = pytz.tzfile.build_tzinfo('local', handle)
    return tz

reveals that two more tests fail:

======================================================================
ERROR: test_timezone_addition_and_dir_correction (aiida.backends.tests.manage.backup.test_backup_script.TestBackupScriptUnit)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/leopold/Personal/Postdoc-MARVEL/repos/aiida/aiida_rmq/aiida/backends/tests/manage/backup/test_backup_script.py", line 249, in test_timezone_addition_and_dir_correction
    self._backup_setup_inst._read_backup_info_from_dict(backup_variables)
  File "/Users/leopold/Personal/Postdoc-MARVEL/repos/aiida/aiida_rmq/aiida/manage/backup/backup_base.py", line 140, in _read_backup_info_from_dict
    self._oldest_object_bk = ptimezone(curr_timezone).localize(self._oldest_object_bk)
  File "/Users/leopold/Applications/miniconda3/envs/aiida_rmq_py3/lib/python3.7/site-packages/pytz/__init__.py", line 181, in timezone
    raise UnknownTimeZoneError(zone)
pytz.exceptions.UnknownTimeZoneError: 'local'

======================================================================
FAIL: test_timezone_now (aiida.backends.tests.common.test_timezone.TimezoneTest)
Test timezone.now function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/leopold/Personal/Postdoc-MARVEL/repos/aiida/aiida_rmq/aiida/backends/tests/common/test_timezone.py", line 31, in test_timezone_now
    self.assertGreaterEqual(from_tz, ref - delta)
AssertionError: datetime.datetime(2019, 11, 26, 7, 1, 37, 661575, tzinfo=<DstTzInfo 'local' CET+1:00:00 STD>) not greater than or equal to datetime.datetime(2019, 11, 26, 13, 0, 37, 661565, tzinfo=<UTC>)

I'll look into whether those can be fixed as well.

@ltalirz
Copy link
Member Author

ltalirz commented Nov 26, 2019

While one test was easy to fix, it turns out that what we are doing here

return value.replace(tzinfo=timezone)

is actually not guaranteed to work in general (only for timezones without daylight saving transitions).

For example, on my machine /etc/localtime is CET:

$ zdump /etc/localtime
/etc/localtime  Tue Nov 26 14:46:18 2019 CET

but creating a pytz timezone object from /etc/localtime using the implementation above yields

In [1]: from aiida.common import timezone

In [2]: timezone.get_current_timezone()
Out[2]: <DstTzInfo 'local' CET+1:00:00 STD>

So, somehow pytz.tzfile.build_tzinfo seems to misinterpret the content of /etc/localtime.

I don't think it's my job to fix this, so I will be adding a warning when the timezone returned by get_localtime is local and proceed.

@ltalirz
Copy link
Member Author

ltalirz commented Nov 26, 2019

To summarize - in the end, there was an issue with deleting the timezone setting from django, since django uses the value provided there (or the default, i.e. America/Chicago) to populate the TZ environment variable.

The value of this variable is, in turn, used by tzlocal, i.e. if we delete the line from the django settings, all times that use tzlocal become Chicago time.

As explained here, there might be a way to get around this by manually configuring django, but this involves passing all settings from the settings.py to django.conf.settings.configure and we didn't want to go with this hassle for the time being.

So, in conclusion, for the moment the solution is to throw an exception when tzlocal returns a local timezone, and suggest in the error message to set the TZ environment variable to the appropriate value.

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

Successfully merging a pull request may close this issue.

4 participants