getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
214 stars 48 forks source link

Make avalon.Session available without io.install and sync with os.environ #439

Closed davidlatwe closed 4 years ago

davidlatwe commented 4 years ago

Problem

avalon.Session is empty until io.install(), wouldn't one expect the avalon.Session has the context right after import avalon ?

Because, from my understanding, avalon.Session is the current Avalon environment context, but io.install is meant to establish the database connection. These were different concepts, and the name Session implies that it holds the current Avalon context, one wouldn't know io.install is required in prior.

Next, I am NOT sure whether this should be considered as a problem or not, but after avalon.Session updated from os.environ, one still able to change Avalon environment variables in os.environ but those changes will not reflect to avalon.Session and vise versa.

Maybe they should be synced ?

Proposal

  1. Make avalon.Session available right after import avalon, without io.install.
  2. Make avalon.Session always sync with os.environ

Implementation

I am not sure about proposal 1), since it's related to the schema validation and still pending on #432.

But for proposal 2), maybe this ?

import os
from avalon import io

class _EnvironSync(dict):
    """A dictionary that Sync values with environment

    Always respect values in `os.environ`, and only sync value that
    corresponding key exists in `os.environ`.

    """

    def __getitem__(self, k):
        try:
            v = super(_EnvironSync, self).__getitem__(k)
        except KeyError as e:
            raise e
        else:
            if k in os.environ:
                # Respect the value from os.environ and sync
                v = os.environ[k]
                super(_EnvironSync, self).__setitem__(k, v)
            return v

    def __setitem__(self, k, v):
        if k in os.environ:
            # Only update os.environ when the entry exists.
            # But careful, os.environ only accept string type value.
            os.environ[k] = v
        super(_EnvironSync, self).__setitem__(k, v)

    def get(self, k, d=None):
        try:
            return self[k]
        except KeyError:
            return d

    def setdefault(self, k, d=None):
        try:
            return self[k]
        except KeyError:
            self[k] = d
            return d

    def update(self, *args, **kwargs):
        super(_EnvironSync, self).update(*args, **kwargs)
        for k, v in self.items():
            if k in os.environ:
                # Careful, os.environ only accept string type value
                os.environ[k] = v

session = _EnvironSync(io._from_environment())

Please let me know what you think :)

BigRoy commented 4 years ago
  1. Make avalon.Session available right after import avalon, without io.install.

This makes the import somewhat magical because it would be unclear to the end user as to when exactly the state from the local environment got copied. For example, I'd imagine this to work:

import avalon.api
import os

os.environ["AVALON_ASSET"] = "hero"
avalon.api.install()

Whereas with that change we'd have to delay the import of avalon, which feels odd to me personally.

Just wondering @davidlatwe what in particular did you feel off about Avalon requiring to trigger an install() to have its settings/session available? Were you having issues with actually installing something?

  1. Make avalon.Session always sync with os.environ

If that's the case, why not always just directly query os.environ and disregard api.Session? I believe the end goal was exactly that one could operate changes to the Session without influencing the local environment.

iLLiCiTiT commented 4 years ago

When I first met with avalon I was actually confused that it is called Session. It is not session at all (at least not what they taught me in school) but current context of database i/o helper as you described and it is actually filled with os.environ so my question is: Why to duplicate same data in 2 singleton dictionaries?

davidlatwe commented 4 years ago

what in particular did you feel off about Avalon requiring to trigger an install() to have its settings/session available? Were you having issues with actually installing something?

I notice and start thinking about this was because I was testing something, and I don't require database at the time but I still need to run io.install and wait for the connection to complete before I can access avalon.Session for my test.

Not a big problem to wait for a few seconds, but that's the motivation I forgot to mention, sorry about that.

This makes the import somewhat magical

Good point, you are right.

Why to duplicate same data in 2 singleton dictionaries?

I think Roy just answered it : :point_down:

I believe the end goal was exactly that one could operate changes to the Session without influencing the local environment.

That's the end goal, yes.

The reason I think that they should be synced is to avoid duplicated values in global scope, and one should make an explicit copy if you want to change it.

But this might break the backward compatibility...

Hmmm, I think the only way to solve my original problem and make sense to everyone is like this

def init_session():
    api.Session.update(io._from_environment())

Or making io._from_environment() public ?

But still, feeling a bit danger for letting api.Session be able to change in anywhere and apply to everywhere.

Am I thinking too much ? :/

iLLiCiTiT commented 4 years ago

I think io has installed boolean. Not sure I've undestood original problem but you probably can use that (or add something similar?)

davidlatwe commented 4 years ago

No, that's not what I am looking for, what I am looking is a good way to separate the initiating of api.Session from io.install.

But I am closing this for now, because the benefit could get from the proposal is less then the issues that may produce. Need a much more stronger motivation and much more specific problem.