-
Notifications
You must be signed in to change notification settings - Fork 58
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
Towards better inference: bits → nibbles #3808
base: main
Are you sure you want to change the base?
Conversation
…coordination into set-default-risk-in-model
…coordination into set-default-risk-in-model
…uler from recreating already deleted oois trhough affirmations
…cheduler_from_reacreating_already_deleted_oois_through_affirmations' into feature/nibbles
This reverts commit 1b4aed6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I've provided mostly Python related feedback since I missed the discussion about Nibbles. I'd love to know more about it, why is it better than bits, and what are the main differences? Perhaps we can meet offline for coffee soon?
octopoes/nibbles/definitions.py
Outdated
NIBBLES_DIR = Path(__file__).parent | ||
NIBBLE_ATTR_NAME = "NIBBLE" | ||
NIBBLE_FUNC_NAME = "nibble" | ||
logger = getLogger(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now using structlog's logger for logging. See other files for example, but looks pretty much like this:
import structlog
logger = structlog.get_logger(__name__)
octopoes/nibbles/definitions.py
Outdated
return False | ||
|
||
def __hash__(self): | ||
return hash(str(self.ooi_type) + self.relation_path if self.relation_path else "\0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be tricky, since both instances with an empty or unset relation_path
could potentially yield the same hash. I suggest using the following:
return hash(str(self.ooi_type) + self.relation_path if self.relation_path else "\0") | |
return hash(bytes(self.ooi_type) + bytes(self.relation_path)) |
return hash(str(self.ooi_type) + self.relation_path if self.relation_path else "\0") | ||
|
||
|
||
class NibbleDefinition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering, what is the reason this isn't implemented as an e.g. Pydantic class but instead as a POJO-like class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow the Pydantic class does not work well with the importlib yielding the payload... not sure why but it fixed the issues so I moved on -- perhaps hoping one day you would fix it ;)
octopoes/nibbles/runner.py
Outdated
U = TypeVar("U") | ||
|
||
|
||
def otype(ooi: OOI) -> type[OOI]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def otype(ooi: OOI) -> type[OOI]: | |
def object_type(ooi: OOI) -> type[OOI]: |
or ooi_type
octopoes/nibbles/runner.py
Outdated
return type_by_name(ooi.get_ooi_type()) | ||
|
||
|
||
def mergewith(func: Callable[[set[T], set[T]], set[T]], d1: dict[U, set[T]], d2: dict[U, set[T]]) -> dict[U, set[T]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def mergewith(func: Callable[[set[T], set[T]], set[T]], d1: dict[U, set[T]], d2: dict[U, set[T]]) -> dict[U, set[T]]: | |
def merge_with(func: Callable[[set[T], set[T]], set[T]], d1: dict[U, set[T]], d2: dict[U, set[T]]) -> dict[U, set[T]]: |
|
||
|
||
def dummy(network: Network): | ||
global NMAX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
global NMAX |
if os.environ.get("CI") != "1": | ||
pytest.skip("Needs XTDB multinode container.", allow_module_level=True) | ||
|
||
NMAX = 13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a more descriptive name here? I'm guessing it has to do with the network name length, so maybe MAX_NAME_LENGTH
? (or MAX_NETWORK_NAME_LENGTH
)
octopoes/nibbles/runner.py
Outdated
|
||
def _cleared(self, ooi: OOI, valid_time: datetime) -> bool: | ||
ooi_level = self.scan_profile_repository.get(ooi.reference, valid_time).level.value | ||
target_nibbles = list(filter(lambda x: type(ooi) in x.signature, self.nibbles)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since any
works on iterables, you don't have to create a list out of the filter object, since the filter object is also an iterable
target_nibbles = list(filter(lambda x: type(ooi) in x.signature, self.nibbles)) | |
target_nibbles = filter(lambda x: type(ooi) in x.signature, self.nibbles) |
octopoes/nibbles/runner.py
Outdated
retval: dict[OOI, dict[str, set[OOI]]] = {} | ||
blockset = set(stack) | ||
self.objects_by_type_cache = {} | ||
if self._cleared(stack[-1], valid_time): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if stack
can be empty, but if not it should probably be checked before using it
octopoes/nibbles/runner.py
Outdated
return any(nibble.min_scan_level < ooi_level for nibble in target_nibbles) | ||
|
||
def infer(self, stack: list[OOI], valid_time: datetime) -> dict[OOI, dict[str, set[OOI]]]: | ||
retval: dict[OOI, dict[str, set[OOI]]] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retval: dict[OOI, dict[str, set[OOI]]] = {} | |
return_value: dict[OOI, dict[str, set[OOI]]] = {} |
or maybe
retval: dict[OOI, dict[str, set[OOI]]] = {} | |
inference: dict[OOI, dict[str, set[OOI]]] = {} |
Quality Gate failedFailed conditions |
Changes
Bits → Nibbles
Issue link
N/A
Demo
T.B.D.
QA notes
T.B.D.
Code Checklist
.env
changes files if required and changed the.env-dist
accordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.