-
Notifications
You must be signed in to change notification settings - Fork 156
Feature/service agent #87
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
base: Feature/AI-Suggestion
Are you sure you want to change the base?
Changes from all commits
0e5153b
e217d68
7fa276c
55c4d8b
beb91a7
e5de1ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,6 +174,7 @@ GEM | |
| faraday-net_http (3.4.0) | ||
| net-http (>= 0.5.0) | ||
| ffi (1.17.1-aarch64-linux-gnu) | ||
| ffi (1.17.1-arm64-darwin) | ||
| ffi (1.17.1-x86_64-linux-gnu) | ||
| fugit (1.11.1) | ||
| et-orbi (~> 1, >= 1.2.11) | ||
|
|
@@ -281,6 +282,8 @@ GEM | |
| nio4r (2.7.4) | ||
| nokogiri (1.18.7-aarch64-linux-gnu) | ||
| racc (~> 1.4) | ||
| nokogiri (1.18.7-arm64-darwin) | ||
| racc (~> 1.4) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto above comment
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The nokogiri (arm64-darwin) entry and its dependency on racc were added automatically by Bundler when resolving native extensions on macOS ARM, so this wasn’t manually introduced. |
||
| nokogiri (1.18.7-x86_64-linux-gnu) | ||
| racc (~> 1.4) | ||
| numerizer (0.1.1) | ||
|
|
@@ -559,6 +562,7 @@ GEM | |
|
|
||
| PLATFORMS | ||
| aarch64-linux | ||
| arm64-darwin-23 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and this one
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, the arm64-darwin-23 entry in the PLATFORMS section was added automatically by Bundler when generating the lockfile on macOS ARM. It is not manually introduced as Bundler records the platform so native gems like Nokogiri/ffi can be resolved correctly across different environments. |
||
| x86_64-linux | ||
|
|
||
| DEPENDENCIES | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -519,4 +519,15 @@ class TasksApi < Grape::API | |
| true | ||
| end | ||
|
|
||
| # effort prediction endpoint for task.. | ||
| desc 'Predict effort for a task' | ||
| params do | ||
| requires :features, type: Array[Float], desc: 'Feature values' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to include ID for task getting prediction
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the moment we are using a dummy model that just accepts 10 inputs and returns a score, so we haven’t tied predictions to specific tasks yet. Once we train and integrate the real model (regression or NLP), we will update the API contract to include task_id (and possibly unit_id) so predictions can be persisted against the correct records. For now, the dummy model is just scaffolding. |
||
| end | ||
| post :predict_effort do | ||
| features = params[:features] | ||
| prediction = EffortPredictionService.predict(features) | ||
| { predicted_effort: prediction } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are we doing with the result of the prediction? we should be saving it somewhere
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing it out — at the moment we are just returning the prediction as a transient value. Once we move beyond the dummy model and integrate the real predictor, we will persist the results in the DB (likely task_definitions) so they can be reused on dashboards and for comparative analysis. For now, I will keep this endpoint simple and return the score, but we will create a follow‑up ticket to implement persistence. |
||
| end | ||
|
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,4 +49,28 @@ def index | |
| rescue MyException => e | ||
| render json: e.message, status: e.status | ||
| end | ||
|
|
||
| # prediction effort function | ||
|
|
||
| protect_from_forgery with: :null_session # allow API POST without CSRF token | ||
| # skip_before_action :verify_authenticity_token, only: [:predict_effort] | ||
|
|
||
| # POST /tasks/predict_effort | ||
| def predict_effort | ||
| features = params[:features] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how do we know what task we are predicting effort for? we dont seem to be identifying tasks in any way here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the moment we are using a dummy model that just accepts 10 inputs and returns a score, so we haven’t tied predictions to specific tasks yet. Once we train the real model (regression or NLP), we’ll likely need to pass a task_id (and possibly unit_id) so predictions can be persisted against the correct records. Happy to keep this as a follow‑up discussion, but for now the dummy model is just for scaffolding. |
||
|
|
||
| if features.blank? | ||
| render json: { error: "Features parameter is required" }, status: :bad_request | ||
| return | ||
| end | ||
|
|
||
| prediction_value = EffortPredictionService.predicted_effort(features) | ||
|
|
||
| if prediction_value | ||
| render json: { predicted_effort: prediction_value } | ||
| else | ||
| render json: { error: "Prediction failed" }, status: :internal_server_error | ||
| end | ||
| end | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to think about if we need this in the task_downloads_controller at all, it doesn't seem like the correct place?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that effort prediction doesn’t really belong in task_downloads_controller. For now I will leave it here to keep the dummy model scaffolding simple, but once we finalize the real model and persistence strategy, we will move the endpoint into a more appropriate controller (likely tasks_controller or a dedicated predictions_controller). |
||
|
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # app/services/effort_prediction_service.rb | ||
|
|
||
| require 'net/http' | ||
| require 'json' | ||
|
|
||
| class EffortPredictionService | ||
| TORCHSERVE_URL = ENV.fetch("TORCHSERVE_URL", "http://effort-predictor:8080/predictions/effort-predictor") | ||
|
|
||
| def self.predict(features) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right now the features argument doesn't seem to be defined any where, it could be good to define the payload a bit clearer for example Features = Struct.New(:arg1, :arg2) and predict can take in this type. I'm not great with Ruby but something like this should hopefully be possible
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks — agreed that passing a raw array of features isn’t very clear. For now, since we are scaffolding with a dummy model, I will keep the array, but once we finalize the real model inputs we will define a Features struct (or similar) so the payload is explicit and easier to maintain. |
||
| uri = URI(TORCHSERVE_URL) | ||
| headers = { | ||
| "Content-Type" => "application/json", | ||
| "Authorization" => "Bearer #{ENV.fetch('TORCHSERVE_INFERENCE_KEY', nil)}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nil default serves no purpose here, apparently torchserve will work without a key and its not being handled in the py handler anyway so this will still work im guessing, begs the question is the auth token required at all if its only ever going to be used in a closed network and isnt exposed to the public? Im not sure of the security implications here but worth thinking about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this sort of things usually comes from the clients request, so its better off here extracing the auth token from the original request, checking if the key is valid before passing on the payload to your predictor service, right now anyone that hits this endpoint will be able to hit the torchserve service There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so there needs to be more aauth work done around this feature that we could create a new ticket for
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did try disabling the auth key earlier, but TorchServe caused issues when running without it. That’s why I have kept the inference key in place for now. I agree that in a closed network the security implications are different, and if we do want authentication it should come from the client request and be validated before forwarding to TorchServe. Let us create a follow‑up ticket to design proper auth around this feature. |
||
| } | ||
| body = { features: features }.to_json | ||
|
|
||
| response = Net::HTTP.post(uri, body, headers) | ||
|
|
||
| if response.is_a?(Net::HTTPSuccess) | ||
| parsed = begin | ||
| JSON.parse(response.body) | ||
| rescue StandardError | ||
| response.body | ||
| end | ||
| parsed.is_a?(Hash) ? parsed["predicted_effort"] || parsed.values.first : parsed | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any other useful information that could be returned from this function
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now we are only returning the predicted effort value, but we could expose more context including raw output, confidence scores, model metadata, timestamp etc. For the dummy model I have kept it minimal, but once we integrate the real model we can expand the response to include these fields so consumers have richer information to work with. |
||
| else | ||
| Rails.logger.error("TorchServe error: #{response.code} #{response.body}") | ||
| nil | ||
| end | ||
| end | ||
|
|
||
| # Added a new helper method | ||
| def self.predicted_effort(features) | ||
| result = predict(features) | ||
| case result | ||
| when Array | ||
| result.first | ||
| when Hash | ||
| result["predicted_effort"] || result.values.first | ||
| else | ||
| result | ||
| end | ||
| end | ||
|
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import torch | ||
| import torch.nn as nn | ||
|
|
||
| class EffortPredictor(nn.Module): | ||
| def __init__(self, input_dim): | ||
| super(EffortPredictor, self).__init__() | ||
| self.fc = nn.Linear(input_dim, 1) | ||
|
|
||
| def forward(self, x): | ||
| return self.fc(x) | ||
|
|
||
| # Create a dummy model with 10 input features | ||
| model = EffortPredictor(input_dim=10) | ||
|
|
||
| # Save its state dict as effort_model.pth | ||
| torch.save(model.state_dict(), "effort_model.pth") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
|
|
||
| import json | ||
| import torch | ||
| from ts.torch_handler.base_handler import BaseHandler | ||
|
|
||
| class EffortRegressionHandler(BaseHandler): | ||
| def postprocess(self, data): | ||
| if isinstance(data, torch.Tensor): | ||
| return {"predicted_effort": float(data.item())} | ||
| return {"predicted_effort": data} | ||
|
|
||
| def handle(self, data, context): | ||
| try: | ||
| # Extract request body | ||
| body = data[0].get("body") | ||
|
|
||
| features = body["features"] # no json.loads | ||
|
|
||
| # Convert to tensor | ||
| tensor = torch.tensor(features).float().unsqueeze(0) | ||
|
|
||
| # Run model | ||
|
|
||
| output = max(0.0, self.model(tensor).item()) # to have positive output | ||
|
|
||
|
|
||
| # Return JSON response | ||
| return [json.dumps({"predicted_effort": output})] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this is returning a json string inside a list, you probably only need to return {"predicted_effort": output} here (the object itself)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agreed that returning json.dumps wraps the JSON string unnecessarily. For now this was needed to get curl responses working with the dummy model, but once we integrate the trained model we will update the handler to return the object itself ({"predicted_effort": output}) so TorchServe can serialize it cleanly. That will also give us flexibility to include additional fields like confidence or task_id. |
||
|
|
||
| except Exception as e: | ||
| return [json.dumps({"error": str(e)})] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, will make it easier on the ruby side to parse the response
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing this out — I agree that returning a dict directly ({"predicted_effort": output}) would be cleaner and easier to parse. At the moment we are using a dummy model, and wrapping the response in json.dumps was needed to get curl responses working reliably. Once we integrate the trained model and have more structured outputs (such as effort, confidence, task_id etc.), we will update the handler to return the object itself so TorchServe can serialize it cleanly. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| { | ||
| "management": { | ||
| "key": "ZT_SKghL", | ||
| "expiration time": "2026-04-13T15:16:21.380749386Z" | ||
| }, | ||
| "inference": { | ||
| "key": "AJVoXRTf", | ||
| "expiration time": "2026-04-13T15:16:21.380728803Z" | ||
| }, | ||
| "API": { | ||
| "key": "hIvWRijq" | ||
| } | ||
| } | ||
|
Comment on lines
+1
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. committing API keys is generally bad practice, if your not worried about it it should be fine i guess
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This key_file.json is auto‑generated by TorchServe for API key management (management, inference, and API keys). It changes every time we run our containers, so committing it is not necessary and could expose sensitive values. We will utilise the inference key via environment variables instead, and add this file to .gitignore so it is excluded from source control. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import torch | ||
| import torch.nn as nn | ||
|
|
||
| class EffortPredictor(nn.Module): | ||
| def __init__(self, input_dim=10): # initialise with a default value | ||
| super(EffortPredictor, self).__init__() | ||
| self.fc = nn.Linear(input_dim, 1) | ||
|
|
||
| def forward(self, x): | ||
| return self.fc(x) | ||
|
|
||
| def get_model(): | ||
| model = EffortPredictor(input_dim=10) # adjust to your features | ||
| model.load_state_dict(torch.load("effort_model.pth")) | ||
| model.eval() | ||
| return model |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| #!/bin/bash | ||
| torch-model-archiver \ | ||
| --model-name effort-predictor \ | ||
| --version 1.0 \ | ||
| --model-file models/effort_model.py \ | ||
| --serialized-file effort_model.pth \ | ||
| --handler handlers/effort_regression_handler.py \ | ||
| --export-path model_store \ | ||
| --force |
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.
is this dependency required
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.
The ffi dependency was pulled in when fixing Rubocop style offenses — the build was failing without it on ARM64, so Bundler added the platform‑specific version to Gemfile.lock. It wasn’t manually added, but required indirectly by other gems likely nokogiri.