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

adding 'id' to block, making 'name' optional, having 'id' play former name-uniqueness role #115

Merged
merged 3 commits into from Mar 2, 2018

Conversation

Projects
None yet
2 participants
@f1401martin
Copy link
Contributor

commented Feb 23, 2018

The idea is to have the framework clean of any backwards compatibility issues, and deal with backwards compatibility in the core, a proof of concept that I tested successfully having the core load old projects while using this framework

@f1401martin f1401martin requested a review from mattdodge Feb 23, 2018

@mattdodge
Copy link
Member

left a comment

A few changes but two overall questions.

  1. My assumption is that each block config will have its own ID. They will be unique per config, not per block class or per block instance. Is that correct? If so, I would think that ID would be included in the configuration of the block somehow and not generated by the block itself. It would likely be generated by the API when the block config was created or maybe by the client creating the block (e.g., system designer).
  2. I don't understand the backwards compatibility plan. This is definitely not backwards compatible, specifically the block routing is not. Is this going to be a 3.0 bump? It doesn't seem like it needs to be but I don't understand how the core can "absorb" the backwards compatibility.
@@ -47,6 +50,7 @@ def __init__(self, status_change_callback=None):

super().__init__(status_change_callback=status_change_callback)

self.id = uuid4().hex

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 23, 2018

Member

Why does the block class ever create its own ID? I would think it would be passed in via the context like other block properties.

@@ -85,7 +89,7 @@ def configure(self, context):
# verify that block properties are valid
self.validate()

self.logger = get_nio_logger(self.name())
self.logger = get_nio_logger(self.id())

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 23, 2018

Member

Is there any reason for this? I would probably prefer to have the name of my block be in the logs (if it is set) rather than a long ID.

This comment has been minimized.

Copy link
@f1401martin

f1401martin Feb 26, 2018

Author Contributor

we could have both I guess, the 'id' is the unique identifier though, blocks can have the same 'name' or none at all

@@ -27,13 +28,13 @@ def get_test_modules(self):
def test_saves_properly(self):
""" Tests that the mixin saves the right values """
block = PersistingBlock()
self.configure_block(block, {"name": "test_block"})
self.configure_block(block, {})

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 23, 2018

Member

I would think we need to configure the block with an ID

This comment has been minimized.

Copy link
@f1401martin

f1401martin Feb 26, 2018

Author Contributor

Currently it generates its own id but it is not set in stone, see comment...

@@ -58,7 +58,7 @@ def _block_defines_input_id(self, block):
return True
else:
raise InvalidProcessSignalsSignature(
"Block {} signature is invalid".format(block.name()))
"Block {} signature is invalid".format(block.id()))

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 23, 2018

Member

I'm actually thinking for log messages and errors and all of that the name would still be useful. Maybe we include both? @hansmosh what do you think?

This comment has been minimized.

Copy link
@f1401martin

f1401martin Feb 26, 2018

Author Contributor

Same as former comment, my thinking is that it should include both.

@@ -23,7 +23,7 @@ class BlockExecution(PropertyHolder):
This information is parsed/used by the Router which is able then, based on
the sending block, to forward signals to its receivers
"""
name = StringProperty(title="Name")
id = StringProperty(title="Id")

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 23, 2018

Member

No more service names!?!

This comment has been minimized.

Copy link
@f1401martin

f1401martin Feb 26, 2018

Author Contributor

This 'id' refers refers to a block ('id') in the service execution, the service name is kept intact

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 28, 2018

Member

Oh yeah, my bad. Saw this was in the service/base.py and assumed it was the base service class. This makes sense

@f1401martin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

  1. I found it restrictive to impose on every API using nio to have to include an id when a default could be provided internally, but don't feel strongly about it since it may have confusing side-effects.
  2. Every configuration is loaded through the core, the core will detect the config format it is reading and adjust it accordingly, for example, if the core sees that the execution, or any other part of the configuration, does not contain 'id' (thus contains 'name') it will patch the configuration with expected format

@mattdodge mattdodge changed the base branch from master to v3.0.0 Feb 26, 2018

@mattdodge

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

After discussion with Franky, here's a summary:

  1. This will require a bump of the framework to 3.0.0. The backwards compatibility of existing service/block configs can be handled by the core but we don't want an old core/binary using the new framework and not being able to work. So I changed the base of this branch to 3.0 so we can work off of that until it's ready.
  2. As far as logging and error messages go, I propose we use a new "label" method to do so. It can use the ID if no name is set. Here is a proposed method:
def label(self, include_id=False):
    if self.name():
        if include_id:
            return "{}-{}".format(self.name(), self.id())
        else:
            return self.name()
    return self.id()
  1. We will not generate block IDs in the constructor anymore. If a new block configuration is provided to the API without an included ID then the API handler can generate one. Blocks should never generate their own though. An ID is a required property of a block and must be passed in the block context.
  2. Need to investigate the implications of this on the existing concept of a "block alias" within a service. If the same block configuration is used multiple times within a service then each instance of that block gets an alias. That code was in place before and I think is still there. In that case the alias should be used for logging and persistence. This PR should not remove the ability to share block configs within a service or across services. If that is something that we are considering we should discuss and open a new PR for it.
@f1401martin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

Verified No4, it works as expected, logging/persistence are logged/saved under alias id

@mattdodge
Copy link
Member

left a comment

New label method looks good. Just a couple suggestions where to change the use of it, but then I think we're good to go

@@ -85,7 +86,7 @@ def configure(self, context):
# verify that block properties are valid
self.validate()

self.logger = get_nio_logger(self.name())
self.logger = get_nio_logger(self.label(True))

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 28, 2018

Member

Would prefer the logger have no ID if a name is specified. This will keep logs cleaner and if they don't specify a name it will still have the ID. The only time the ID would help would be if they somehow had two block configs with the same name but different ID. Unlikely scenario

is loaded, all persisted values are examined against loaded
item, if it exists, such value is then set as an attribute
in the block instance
"""
self.logger.debug("Loading from persistence")
# load whole item from persistence
item = self._persistence.load(self.name(), default={})
item = self._persistence.load(self.label(include_id=True), default={})

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 28, 2018

Member

We should probably just persist off the id only, right? It's rarely read by humans and by using the label we would lose the persistence if the block was renamed.

This comment has been minimized.

Copy link
@f1401martin

f1401martin Feb 28, 2018

Author Contributor

good call!

@@ -354,15 +354,15 @@ def notify_signals(self, block, signals, output_id):
elif self.status.is_set(RunnerStatus.stopped):
self.logger.warning("Block Router is stopped, discarding signal"
" notification from block: {}".
format(block.name()))
format(block.id()))

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 28, 2018

Member

Use block.label() in all of these warning and exception messages instead of block.id()

@@ -23,7 +23,7 @@ class BlockExecution(PropertyHolder):
This information is parsed/used by the Router which is able then, based on
the sending block, to forward signals to its receivers
"""
name = StringProperty(title="Name")
id = StringProperty(title="Id")

This comment has been minimized.

Copy link
@mattdodge

mattdodge Feb 28, 2018

Member

Oh yeah, my bad. Saw this was in the service/base.py and assumed it was the base service class. This makes sense

@mattdodge mattdodge merged commit 6aa1d0a into v3.0.0 Mar 2, 2018

1 check passed

n.io Platform/nio Framework Build #494 succeeded in 8.5 sec
Details

@mattdodge mattdodge deleted the name_to_id branch Mar 2, 2018

@aaronranard aaronranard changed the title NIO-1053, adding 'id' to block, making 'name' optional, having 'id' play former name-uniqueness role adding 'id' to block, making 'name' optional, having 'id' play former name-uniqueness role Apr 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.