change from `present?` (Rails) to native Ruby check #57

mculp · 2024-09-10T02:20:00Z

I'm not sure where present? (a method defined in Rails) is being pulled in from, but for me, it's not:

NoMethodError: undefined method `present?' for nil (NoMethodError)

        elsif multi_tenant.present?
                          ^^^^^^^^^

I wonder if a truthiness check is good enough: elsif multi_tenant

or do we need to explicitly check that it's not empty, similar to present?

I'm not sure where `present?` (a method defined in Rails) is being pulled in from, but for me, it's not: ```sh NoMethodError: undefined method `present?' for nil (NoMethodError) elsif multi_tenant.present? ^^^^^^^^^ ``` I wonder if `elsif multi_tenant` is good enough, or do we need to explicitly check that if it has an `empty?` method, that the `empty?` method returns false (similar to what `present?` does)

andreibondarev · 2024-09-10T02:25:35Z

lib/weaviate/schema.rb

@@ -45,7 +45,7 @@ def create(
        req.body["properties"] = properties unless properties.nil?
        if multi_tenant.is_a?(Hash)
          req.body["multiTenancyConfig"] = multi_tenant
-        elsif multi_tenant.present?
+        elsif multi_tenant && !(multi_tenant.respond_to?(:empty?) && multi_tenant.empty?)


@mculp If you look at the method signature: multi_tenant: nil, so it's either nil or "whatever the user passes in" (which should be true.

I'm wondering if we should just change the method signature to:

def create( multi_tenant: false

and then in the method itself, always set the multiTenancyConfig to:

req.body["multiTenancyConfig"] = {enabled: multi_tenant}

It seems like this code is allowing a Hash to be passed in and set instead of { enabled: true }

if multi_tenant.is_a?(Hash) req.body["multiTenancyConfig"] = multi_tenant

I think because this is valid, according to the docs:

multi_tenancy_config=Configure.multi_tenancy( enabled=True, auto_tenant_creation=True )

so looks like there are two options here we need to handle

wdyt about:

def create( multi_tenant: false, auto_tenant_creation: false, # ... req.body["multiTenancyConfig"] = { enabled: multi_tenant } req.body["multiTenancyConfig"].merge!(auto_tenant_creation: auto_tenant_creation) if auto_tenant_creation

@mculp I don't see the auto_tenant_creation option. I'm not sure what the Python client implement but this gem just interfaces with the REST API.

https://weaviate.io/developers/weaviate/config-refs/schema

@andreibondarev I think it was added in 1.25. Check the curl example here:

https://weaviate.io/developers/weaviate/manage-data/multi-tenancy#automatically-add-new-tenants

Yeah, if you'd like to change it -- let's do it!

I made a couple real HTTP calls from the client and checked that this did automatically create tenants when set up properly. I think it'd be nice to use something like VCR for recording requests and responses for use in tests. I started to set it up in this PR, but decided to limit the changeset here as much as I could.

I left the is_a?(Hash) check in there for backwards compatibility. It seems like whoever added this is using the code like:

schema.create({ class_name: "Question", multi_tenant: { enabled: true, autoTenantCreation: true, autoTenantActivation: true } })

I added a shortcut:

schema.create({ class_name: "Question", multi_tenant: true, auto_tenant_creation: true, auto_tenant_activation: true })

These result in the same multiTenancyConfig, and I added a spec for the new case.

It seems a little less than optimal, but I didn't want scope to creep too far. I'm mainly here about the .present? issue.

That said, while I was testing, I found one more instance of .blank? (another Rails method, the inverse of .present?) and just changed it to .nil? (Ruby method).

While I was writing this code, I had a lot of ideas for refactoring/simplification, but wanted to go ahead and ship this before moving on to anything else.

mculp · 2024-09-13T06:04:29Z

spec/weaviate/schema_spec.rb

+      before do
+        @captured_request = nil
+        allow_any_instance_of(Faraday::Connection).to receive(:post) do |_, path, &block|
+          expect(path).to eq("schema")
+          req = OpenStruct.new(body: {})
+          block.call(req)
+          @captured_request = req
+          response
+        end
+      end


VCR or WebMock would simplify this a ton, but this is essentially grabbing the req object yielded to Faraday so we can check that multiTenancyConfig is set up correctly.

mculp · 2024-09-13T18:03:04Z

whoops, I’ll fix the rubocop issues tonight

README.md

andreibondarev · 2024-09-14T00:16:08Z

lib/weaviate/schema.rb

@@ -27,6 +27,8 @@ def create(
      description: nil,
      properties: nil,
      multi_tenant: nil,
+      auto_tenant_creation: nil,
+      auto_tenant_activation: nil,


I feel like it would be simpler to just default to false as that's what Weaviate itself does:

def create( ... multi_tenant: false, auto_tenant_creation: false, auto_tenant_activation: false

And we'd just always set those values:

req.body["multiTenancyConfig"] = {enabled: multi_tenant, autoTenantCreation: auto_tenant_creation, autoTenantActivation: auto_tenant_activation}

I was thinking that it was strange that multi_tenant was the only arg that didn't match the weaviate arg, but didn't want to make a whole lot of changes in this PR.

MultiTenancyConfig = Struct.new(:enabled, :autoTenantCreation, :autoTenantActivation) do def initialize(enabled: false, autoTenantCreation: false, autoTenantActivation: false) super end end

create( ... multi_tenancy_config: MultiTenancyConfig.new )

Since this can be used in update as well, I think it'd make sense to have a small Struct that defines the options.

For now, I'll go with your suggestion, because we are way off track of fixing the present? / blank? issue

So, just to be sure, you want me to blow away the existing code for this? I was trying to keep it backwards compatible.

Yes, please!

I think it's okay, I'll release a 0.10 to signify the breaking change here.

andreibondarev · 2024-09-17T02:41:30Z

Great job, thank you!

mculp changed the title ~~change from present? to ruby-only check~~ change from present? (Rails) to native Ruby check Sep 10, 2024

andreibondarev reviewed Sep 10, 2024

View reviewed changes

allow autoTenantCreation

c44ecc6

mculp commented Sep 13, 2024

View reviewed changes

add autoTenantActivation and update README

72c9963

andreibondarev reviewed Sep 14, 2024

View reviewed changes

README.md Show resolved Hide resolved

andreibondarev reviewed Sep 14, 2024

View reviewed changes

Update schema.create options

48102ef

andreibondarev merged commit 45f48c9 into patterns-ai-core:main Sep 17, 2024
6 checks passed

change from present? (Rails) to native Ruby check #57

change from present? (Rails) to native Ruby check #57

Uh oh!

Conversation

mculp commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreibondarev Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreibondarev Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mculp Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mculp commented Sep 13, 2024

Uh oh!

Uh oh!

andreibondarev Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mculp Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andreibondarev commented Sep 17, 2024

Uh oh!

Uh oh!

change from `present?` (Rails) to native Ruby check #57

change from `present?` (Rails) to native Ruby check #57

mculp commented Sep 10, 2024 •

edited

Loading

andreibondarev Sep 10, 2024 •

edited

Loading

andreibondarev Sep 10, 2024 •

edited

Loading

mculp Sep 13, 2024 •

edited

Loading

andreibondarev Sep 14, 2024 •

edited

Loading

mculp Sep 17, 2024 •

edited

Loading