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

Add job name prefix just before job submit. #3887

Merged

Conversation

euler-room
Copy link
Contributor

@euler-room euler-room commented Oct 21, 2024

This fix checks job_name for a path before prepending said path to it.

Findings:

  • This issue does not appear to be a data integrity problem as the correct job name is saved in data/projects/<project_id>/.ondemand/scripts/<launcher_id>/cache.json.
  • The path prepending the job name only gets duplicated when auto_job_name is not set to "fixed".

Potential issue:

  • This fix feels like a band aid to a very elusive issue.

@johrstrom
Copy link
Contributor

This fix feels like a band aid to a very elusive issue.

I think I see what's going on here.

  • You save a name, say abc123
  • This get serialized as "#{prefix}/abc123"
  • This gets read when you're submitting the job, applying the prefix again making "#{prefix}/#{prefix}/abc123"

So I think the fix is likely leaving value alone and only applying the prefix in submit. This way we don't serialize the prefix at all, it's always abc123 and we apply the prefix in-situ on top of the value while submitting the job.

@euler-room
Copy link
Contributor Author

euler-room commented Oct 22, 2024

I'm still a bit baffled by this. For the following example, fixed_job_name is displaying correctly and has_job_name is the case that displays a duplicated path in the expanded job viewer:

Fixed Job Name:

form.yml

  ---
  title: FIXED-JOB-NAME  
  created_at: 1729452595  
  .
  .
  .
  auto_job_name:
    value: fixed-job-name
    fixed: true
    label: Job Name
    help: ''
    required: false

cache.json

    {
    "auto_accounts": "pzs0714",
    "auto_scripts": "/users/PZS0714/gbuchanan/ondemand/src/ondemand/apps/dashboard/data/projects/0ulyqeri/blast.sh",
    "auto_batch_clusters": "ascend",
    "auto_job_name": "ondemand/dev/dashboard/project-manager/fixed-job-name"
  }

Has Job Name:

form.yml

  ---
  title: HAS-JOB-NAME
  created_at: 1729452582
  .
  .
  .
  auto_job_name:
    value: has-job-name
    label: Job Name
    help: ''
    required: false

cache.json

  {
    "auto_accounts": "pzs0714",
    "auto_scripts": "/users/PZS0714/gbuchanan/ondemand/src/ondemand/apps/dashboard/data/projects/0ulyqeri/blast.sh",
    "auto_batch_clusters": "ascend",
    "auto_job_name": "ondemand/dev/dashboard/project-manager/has-job-name"
  }

I see no difference in the serialization and yet the name has a duplicated prefix if auto_job_name is not set to fixed. This leads me to believe some sort of normalization will still be needed, possibly to add the prefix to fixed auto_job_name fields when displaying them.

@euler-room
Copy link
Contributor Author

euler-room commented Oct 22, 2024

My solution normalizes what is submitted to the cluster but doesn't address why it needs to be normalized.

@johrstrom
Copy link
Contributor

OK- I actually know what's going on here. We get duplicate prefixes when you press launch with default values (the play button). If you open the form (with the hamburger icon) and submit it that way - it's correct.

You can inspect the HTML of this element and see it's a hidden form with all inputs already populated. You see that it's populated with the prefix.

image

Pressing this button sends this to the controller, which then appends the prefix again.

The form is built from this method, where we're calling value.

https://github.com/OSC/ondemand/blob/b4f6a724575bcf5968675048a84fe9e4eef57e2e/apps/dashboard/app/models/launcher.rb#L111_launcher_buttons.html.erb#L16

So, I think the best solution is just to have a simple value method and format/prefix the job name in-situ right before we submit. This preserves the actual value whenever we serialize to yml or when building HTML elements.

--- a/apps/dashboard/app/lib/smart_attributes/attributes/auto_job_name.rb
+++ b/apps/dashboard/app/lib/smart_attributes/attributes/auto_job_name.rb
@@ -17,7 +17,7 @@ module SmartAttributes
       # Defaults to ondemand/[dev,sys]/projects
       # @return [String] attribute value
       def value
-        job_name(opts[:value] || 'Project Manager Job')
+        opts[:value] || 'Project Manager Job'
       end
 
       def widget
@@ -32,7 +32,7 @@ module SmartAttributes
       # @param fmt [String, nil] formatting of hash
       # @return [Hash] submission hash
       def submit(*)
-        { script: { job_name: value } }
+        { script: { job_name: job_name(value) } }
       end

@euler-room
Copy link
Contributor Author

This makes sense. Thank you!

@euler-room euler-room changed the title Check for path in job name before finalizing it. Add job name prefix just before job submit. Oct 24, 2024
@johrstrom johrstrom merged commit 569be8a into OSC:master Oct 24, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants