-
Notifications
You must be signed in to change notification settings - Fork 1
[Ruby+Docker] Fix production environment issues #246
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
Conversation
…into feature/production
p3rcypj
left a comment
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.
Some end-of-file (EOF) changes occurred because the default code formatter was not installed on the machine I was using to modify the files.
|
Docker Compose configurations and environment variables may change in the coming weeks as our new needs become clearer. |
tokland
left a comment
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.
In-line comments:
| iedb: onF(proteinId, proteinId => getJSON(`${bioUrl}/api/annotations/IEDB/Uniprot/${proteinId}`, increasedTimeout)), | ||
| elmdbUniprot: onF(proteinId, proteinId => getJSON(`${bioUrl}/api/annotations/elmdb/Uniprot/${proteinId}`, increasedTimeout)), | ||
| mutagenesis: onF(proteinId, proteinId => getJSON(`${bioUrl}/api/annotations/biomuta/Uniprot/${proteinId}`, increasedTimeout)), | ||
| // BIONOTES |
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.
Was this change about the timeout intended for this infrastructure PR? In any case, for that kind of situations where we repeatedly call a refined version of the function, we can create an in-line abstraction:
const getJSON2 = (url: string) => getJSON(url, { timeout: 100e3 });(if you can find a better name than getJSON2...)
| genomicVariantsCNCB: onF(proteinId, proteinId => getJSON(`${bioUrl}/ws/lrs/features/variants/Genomic_Variants_CNCB/${proteinId}/`)), | ||
| pdbPublications, | ||
| nmrTargets, | ||
| pdbEmdbsEmValidations, |
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.
minor, but lately the consensus is to write nmrTargets: nmrTargets so we can easily navigate to the type OR the value.
| @alignment = nil | ||
| else | ||
| url = BaseUrl+"/api/mappings/Uniprot/ENSEMBL/transcript/"+uniprot_acc | ||
| url = BaseUrl+"api/mappings/Uniprot/ENSEMBL/transcript/"+uniprot_acc |
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.
A more resilient solution is to use a function that performs URI joining, such as join_url(BaseUrl, "/api/mappings/Uniprot/ENSEMBL/transcript", uniprot_acc).
This join_url function would be defined in a global module (which would use URI.join, for example).
|
|
||
| if Rails.env.development? | ||
| skip_before_action :verify_authenticity_token, only: [:home] | ||
| end |
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.
Minor, but this Rails filters have a "if" param, right? skip_before_action :verify_authenticity_token, only: [:home], if: -> { Rails.env.development? }
| identifierName.strip! | ||
| redirect_to ws_path + "/viewer/#/" + identifierName | ||
| ws_path_uri = URI.parse(ws_path) # Note: not sure why rails doesn't let me define ws_path_uri outside | ||
| ws_path_uri.port = Settings.APP_EXT_PORT |
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.
As 1) this action is performed twice, and 2) we have to inspect the code to see what's doing -> create an abstraction with a declarative name
| begin | ||
| _url = URI.parse(url) | ||
| http = Net::HTTP.new(_url.host, _url.port) | ||
| http.use_ssl = (_url.scheme == "https") # Ensure HTTPS is used if needed |
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.
[format] I think Ruby formatters prefer not to add () for this kind of expressions
| File.write(LocalPath+"/"+rand+"_can_seq", uniprot_seq) | ||
| File.write(LocalPath+"/"+rand+"_trans_seq", transcript_seq) | ||
| File.write(LocalPath+rand+"_can_seq", uniprot_seq) | ||
| File.write(LocalPath+rand+"_trans_seq", transcript_seq) |
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.
Same idea that URIs: abstraction or direct File.join
| File.write(LocalPath+rand+"_trans_seq", transcript_seq) | ||
|
|
||
| cmd = "echo \"\n\" | water -asequence "+LocalPath+"/"+rand+"_can_seq -bsequence "+LocalPath+"/"+rand+"_trans_seq -gapopen 50 -gapextend 0 -datafile "+IdentityMatrix+" -aformat3 markx10 -stdout -aglobal3 Y -awidth3 1000000 2> /dev/null" | ||
| cmd = "echo \"\n\" | water -asequence "+LocalPath+rand+"_can_seq -bsequence "+LocalPath+rand+"_trans_seq -gapopen 50 -gapextend 0 -datafile "+IdentityMatrix+" -aformat3 markx10 -stdout -aglobal3 Y -awidth3 1000000 2> /dev/null" |
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.
That code is not ours, so let's keep it as-is to minimize the diffs, but for future code like this, a wiser usage of the quote character and mult-iline joined strings are more readable. For example:
cmd = [
'echo "\n" | water',
"-asequence #{LocalPath}#{rand}_can_seq",
"-bsequence #{LocalPath}#{rand}_trans_seq",
"-gapopen 50",
"-gapextend 0",
"-datafile #{IdentityMatrix}",
"-aformat3 markx10",
"-stdout",
"-aglobal3 Y",
"-awidth3 1000000",
"2> /dev/null"
].join(' ')| # Disable serving static files from the `/public` folder by default since | ||
| # Apache or NGINX already handles this. | ||
| config.serve_static_files = ENV['RAILS_SERVE_STATIC_FILES'].present? | ||
| # config.serve_static_files = true //DEPRECATED |
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.
If it's deprecated, we need to insert the code? if it's something we really need to document, we can write a comment as text. Commented-out code always raises questions to the reader (is this temporal? will we need to uncomment it at some point?)
| # Asset digests allow you to set far-future HTTP expiration dates on all assets, | ||
| # yet still be able to expire them through the digest params. | ||
| config.assets.digest = false | ||
| # config.assets.digest = false |
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 maybe be explicit and set to true? or it's not the same?
📌 References
📝 Implementation
The Django production environment was unable to deploy successfully on either the staging or production machines. Several issues were preventing production deployment. Some of these included:
skip_before_filter→skip_before_action)gem gsl)gem net-ftp)ActiveStorage, even if it is not used)production.rbenvironmentIEDBbionotes.epitope)_footer.html.hamllayout, causing several routes to return HTTP 500 errorsAfter resolving these issues, some route requests were still failing, resulting in HTTP 404 and HTTP 500 errors. This required further troubleshooting, often by "pulling the thread." Most of these issues were related to:
genomicIFrame, aka "GeneViewer")mime.types)//) and the lack of HTTPS supportSome improvements have also been made:
.dockerignore🔥 Testing
For local deployment, neither BWS nor LRS is required anymore. Instead, requests will be redirected to
https://3dbionotes.cnb.csic.es.If redirection to local BWS or LRS is needed for development purposes, copying the Nginx service from
docker-compose.staging.ymltodocker-compose.development.ymland settingBWS_API_PORTandLRS_API_PORT(as referenced in the.env.samplefile) will allow redirection to local services.If only one of the two is needed, modifying
./nginx/nginx.confwill be necessary, but./nginx/nginx.dev.confcan be used as a reference to delegate the unused service tohttps://3dbionotes.cnb.csic.es.